Issue 29330403: Issue #1467, #3397 - Rewrite the map from threads to tabs (Closed)
DescriptionIssue #1467, #3397 - Rewrite the map from threads to tabs
Remove 's_threadInstances' and certain uses of 's_criticalSectionLocal'.
Replace them with a new class that encapsulates a map instance together with
its own mutex. Add unit tests for the new class.
Change where thread entries are inserted from OnWindowStateChanged() to
SetSite().
Replace GetTab(DWORD) to GetTabCurrentThread(), since the original
function was always called with the same argument, the result of
GetCurrentThreadId(). Also, move other calls to this function into a single
class definition.
Patch Set 1 : #Patch Set 2 : rebase only #
Total comments: 67
Patch Set 3 : rebase to current tip / address comments #
Total comments: 9
Patch Set 4 : rebase / fix copyright year #
Total comments: 8
Patch Set 5 : rename some symbols #
Total comments: 6
Patch Set 6 : add comment; new function body #
Total comments: 3
MessagesTotal messages: 20
I've been fiddling at the edges of this for a few days. This version seems good enough to start a proper review.
New patch set is a rebase to current tip. Includes the recent modifications to 'SetSite()'. This review had no attention in its first two weeks. Let's hope that's not the same for the next two weeks.
Update issue description to reference a new defect ticket #3397.
Correctly initialize entries in the map from threads to tabs
https://issues.adblockplus.org/ticket/3397
OK. It's been a month now with no comment at all. It's been a day shy of two weeks since this change set could be directly applied to current tip. Is there something about this change set that makes it particularly hard to review?
On 2015/12/22 12:50:27, Eric wrote: > OK. It's been a month now with no comment at all. It's been a day shy of two > weeks since this change set could be directly applied to current tip. > > Is there something about this change set that makes it particularly hard to > review? That's really not convincing that it does work as expected, see last (2015-11-20 03:03:01 UTC) Oleksandr's comment here https://codereview.adblockplus.org/29319002/. I'm waiting for the comment from Oleksandr.
On 2016/01/26 23:03:16, sergei wrote: > That's really not convincing that it does work as expected, see last (2015-11-20 > 03:03:01 UTC) Oleksandr's comment here > https://codereview.adblockplus.org/29319002/. You are avoiding the question by avoiding comment on the present code and merely referencing comments elsewhere. If you don't think this code works, say so in your own words. If you are unable to make an argument about what's wrong, then you have not contributed to the conversation, but merely stood in the way of progress.
On 2016/01/29 14:08:20, Eric wrote: > On 2016/01/26 23:03:16, sergei wrote: > > That's really not convincing that it does work as expected, see last > (2015-11-20 > > 03:03:01 UTC) Oleksandr's comment here > > https://codereview.adblockplus.org/29319002/. > > You are avoiding the question by avoiding comment on the present code and merely > referencing comments elsewhere. I'm saving my and your time. > > If you don't think this code works, say so in your own words. I can describe it explicitly if it's not clear: I am not sure that adding of the pair of <thread ID, CPluginClass*> into the map may be done in `CPluginClass::SetSite` instead of in `CPluginClass::OnWindowStateChanged`. As I understand from Oleksandr's answer https://codereview.adblockplus.org/29319002/, initializing SetSite can be called from one thread and OnWindowStateChanged is called from another thread. It looks logical that SetSite can be called from some background worker thread and OnWindowStateChanged is called later from UI-thread, especially when many tabs are being opened because no one wants to have a UI-lag because of instantiating of plugins for currently invisible tabs. It's important because we don't want to get wrong (or even dangling pointer to) CPluginClass as well as it affects request blocking mechanism (we call `CPluginClass::GetTab(::GetCurrentThreadId());` from WBPassthruSink::BeginningTransaction). As I told above, now it is not convincing that it does work as expected in this codereview. > If you are unable > to make an argument about what's wrong, then you have not contributed to the > conversation, but merely stood in the way of progress. It was you who asked me to don't keep silence when I would like to firstly know the opinion of others https://codereview.adblockplus.org/5187613258416128/#msg26 On 2015/12/22 14:46:19, Eric wrote: > On 2015/12/22 14:27:07, sergei wrote: > > I'm waiting for the opinion of others regarding class naming. > > You should have said that three weeks ago in response to my rejection of your > naming suggestions. Remaining silent when you think you're asking for something > is a sure way to damage the development process. Please don't do this in the > future.
I have been able to reproduce the case when SetSite and OnWindowsStateChanged are called from different threads, so I don't think this will work, unfortunately. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:35: class SyncMap Isn't this essentially the same as concurrent_unordered_map? https://msdn.microsoft.com/en-us/library/hh750089.aspx Wouldn't we be better of with just using that?
On 2016/02/01 10:53:29, Oleksandr wrote: > I have been able to reproduce the case when SetSite and OnWindowsStateChanged > are called from different threads, so I don't think this will work, > unfortunately. The presence of non-site threads for UI events is one of the reasons for the change. Our current behavior is defective on this point. We are currently adding entries to the thread map in a UI thread, which may or may not be the same as the apartment thread associated with the site. The non-site thread is not actually the thread that's uniquely associated with any single thread. Using a UI thread as a tab thread entry is conceptually wrong. This map is referenced in 'WBPassthruSink::BeginningTransaction()'. This method is always called from the apartment thread for the site (so says Tandetnik). Thus, there's a race condition in the present code. It's triggered when a non-associated UI thread makes the map entry, and subsequently 'BeginningTransaction()' does not find its associated tab. This can cause failure to block certain elements. There are only two other places where this map is used. One is in 'GetStatusBarIcon()', where it's used to determine whether the plugin is disabled or not. Its sole caller has a tab object already whose have is derived from a different map, the one from window handles to tabs. 'GetStatusBarIcon()' then uses the thread map (using the current thread ID) to find a possibly-different or possibly-nonexistent tab. This usually works, simply because IE doesn't always use extra UI threads. Nevertheless, this function should probably be rewritten to accept a tab argument and not reference the thread map at all. The only other way this map is used is to position the notification window to the center of the window. At worst this window won't appear centered in the IE pane. This code has available the tab derived from a window handle, and it should probably use that. Conclusions: 1) We definitely have a defect regarding 'BeginningTransaction()'. 2) We have some probable defects regarding UI elements. Neither needs the thread map. I did not include changes for the UI parts in the present change set. I could certainly post a patch set incorporating these changes. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:35: class SyncMap On 2016/02/01 10:53:28, Oleksandr wrote: > Isn't this essentially the same as concurrent_unordered_map? It's similar in purpose, but that's about it. You might consider replacing member 'idMap' with this class, but it looks like that would be a step backwards. 'concurrent_unordered_map' does not have a member analogous to 'find()'. Instead, you'd have call its 'at()' method and catch the 'out_of_range' exception. Not exactly an improvement. Furthemore, 'concurrent_unordered_map' doesn't even have a concurrency-safe 'erase()' method. Instead, it only has 'unsafe_erase', which is specifically marked as not safe. So it doesn't work even as the underlying container.
All in all, I don't mind about having synchronized map (of course with proper implementation) but it seems we cannot 1) move assignment into SetSite 2) throw if there is already another instance assiciated with the thread. On 2016/02/01 12:14:59, Eric wrote: > Our current behavior is defective on this point. We are currently adding > entries to the thread map in a UI thread, which may or may not be the > same as the apartment thread associated with the site. Could you please explain what you understand under "apartment thread associated with the site"? JIC, we are not using single-threaded apartment model. > The non-site > thread is not actually the thread that's uniquely associated with any > single thread. Could you please explain what the association between threads you are talking about here is? > Using a UI thread as a tab thread entry is conceptually > wrong. I'm not sure about it because it depends on the archutecture. Currently it seems indeed very lucky assumption for us which works in many cases. Otherwise there would not be any way to find current root document URL for the request. > This map is referenced in 'WBPassthruSink::BeginningTransaction()'. This > method is always called from the apartment thread for the site (so says > Tandetnik). Thus, there's a race condition in the present code. It's > triggered when a non-associated UI thread makes the map entry, and > subsequently 'BeginningTransaction()' does not find its associated tab. > This can cause failure to block certain elements. 1.1 Where exactly does he say it? All details are important. 1.2 Anyway, we do observe that sometimes it's called from different threads and it's indeed an issue for us. 2. What you are talking is not a race condition, however there is indeed a possibility for the race condition #3234. > There are only two other places where this map is used. One is in > 'GetStatusBarIcon()', where it's used to determine whether the plugin is > disabled or not. Its sole caller has a tab object already whose have is derived > from a different map, the one from window handles to tabs. 'GetStatusBarIcon()' > then uses the thread map (using the current thread ID) to find a > possibly-different or possibly-nonexistent tab. This usually works, simply > because IE doesn't always use extra UI threads. Nevertheless, this function > should probably be rewritten to accept a tab argument and not reference the > thread map at all. Could you please create an issue for it? https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* Header guard is missed. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* It would be better to rename the file to SyncMap.h https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:3: * Copyright (C) 2006-2015 Eyeo GmbH 2016 https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:21: * A base class for a synchronized map from threads to BHO instances. Either the comment or class should not be so thread-pointer specific. Either it's a template SyncMap or it's a concrete class without any templates. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:34: template<class Key, class T, T nullValue> We don't need `T nullValue`. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:42: std::map<Key, T> idMap; it would be better to call just "map". https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:47: std::mutex mutex; it should has `mutable`. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) What about constant references or more concrete interface? https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) it would be better to call the arguments as "key" and "value". https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; `T` is not necessarily default constructible, it's better to use `emplace` or `insert` here. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:73: auto it = idMap.find(id); Can we do instead `return 1 == idMap.erase(id);`? https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:83: * Returns a non-null pointer if a key of value 'id' is present. This comment does not fit the template class. We don't know whether T is a pointer or not. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:86: T Locate(Key id) This method should be constant method. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:86: T Locate(Key id) I would rather prefer to pass T by reference to use it as a destination for the copy of stored object and return `bool` to indicate whether it's found or not. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:90: return (it != idMap.end()) ? it->second : nullValue; As far as I remember () are not needed here. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:532: s_threadInstances[::GetCurrentThreadId()] = this; Pay attention that we are overriding the value of `CPluginClass*` for `::GetCurrentThreadId()`. But now it does not happen. See also the comment above in SetSite. As it has been said in comments earlier, it looks logical to set the current CPluginClass* from current UI thread. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:67: Why not to put it into anonymous namespace? https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:96: CurrentThreadMap threadMap; Off topic: Still a global variable. When will we stop it?... https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:251: throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map"); That looks suspicious. It can happen that two instances are instantiated in one thread and it should be fine for us. By throwing the exception here, we are basically breaking the functionality for any other instance created using the same thread. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... File test/plugin/InstancesTest.cpp (right): https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:20: typedef SyncMap<int, int, 0> SyncMapOne; What does "One" mean?
On 2016/02/01 15:50:44, sergei wrote: > All in all, I don't mind about having synchronized map (of course with proper > implementation) but it seems we cannot 1) move assignment into SetSite 2) throw > if there is already another instance assiciated with the thread. Both of these are conscious and intentional design decisions. 1) Our current code of adding elements somewhere other than 'SetSite()' is defective. The present code, which moves that insertion into 'SetSite()', corrects a defect. 2) The behavior of IE is that there's a one-to-one relation between IE threads that call 'SetSite()' and BHO instances. It's never the case that there will ever be a second instance associated with a site thread. In such a case where you have a behavioral assumption in the code, you can take one of three kinds of approaches: (a) assume it always true and omit any checking, (b) check the assumption and mask any error, (c) check the assumption and fail on any error. This code uses approach (c), which yields the most robust code. > Could you please explain what you understand under "apartment thread associated > with the site"? JIC, we are not using single-threaded apartment model. "Apartment thread" is a phrase I lifted from Tandetnik. We're talking about IE's threading model, not ours. I don't know what exact thread model IE uses for BHO interaction, but IE has the one-to-one relation between threads and BHO instances that behave as if each BHO runs in a single-threaded apartment. Perhaps that's how IE originally implemented it, perhaps that's how it's still implemented, but regardless we can rely on the behavior that the thread that calls 'SetSite(non-zero)' is the same thread that calls a number of other functions. > Could you please explain what the association between threads you are talking > about here is? See above. > > Using a UI thread as a tab thread entry is conceptually > > wrong. > > I'm not sure about it because it depends on the archutecture. It's wrong because the life cycle of BHO instances is not the same as that for UI widgets. They overlap to a great degree, but they're not the same. It was a very poor design decision to combine them in the first place. It's a decision we haven't extricated ourselves from yet. > > This map is referenced in 'WBPassthruSink::BeginningTransaction()'. This > > method is always called from the apartment thread for the site (so says > > Tandetnik). Thus, there's a race condition in the present code. It's > > triggered when a non-associated UI thread makes the map entry, and > > subsequently 'BeginningTransaction()' does not find its associated tab. > > This can cause failure to block certain elements. > > 1.1 Where exactly does he say it? All details are important. In a comment transcribed inside the definition of 'WBPassthruSink::Switch()'. I don't know the ultimate source for that quotation. > 1.2 Anyway, we do observe that sometimes it's called from different threads and > it's indeed an issue for us. UI functions can be called from separate threads, as can some of the pluggable protocol functions. I've not heard reports that 'BeginningTransaction()' is. > 2. What you are talking is not a race condition, however there is indeed a > possibility for the race condition #3234. Looks like a race condition to me. Depending on which thread in IE wins to race to make a UI call into our code, we get either a site thread or some other worker thread. > Could you please create an issue for it? It's really a part of #3397. That issue currently only speaks directly to initialization, but it's as much about the invariants as anything. We could expand the text of #3397 if you'd like. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/01 15:50:42, sergei wrote: > Header guard is missed. Acknowledged. FYI, it's already present in https://codereview.adblockplus.org/29333107/, where the other class destined for this header is present. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/01 15:50:42, sergei wrote: > It would be better to rename the file to SyncMap.h See https://codereview.adblockplus.org/29333107/. There are two classes related to BHO instances to be defined here. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:3: * Copyright (C) 2006-2015 Eyeo GmbH On 2016/02/01 15:50:42, sergei wrote: > 2016 Done. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:21: * A base class for a synchronized map from threads to BHO instances. On 2016/02/01 15:50:41, sergei wrote: > Either the comment or class should not be so thread-pointer specific. Either > it's a template SyncMap or it's a concrete class without any templates. It's a template class to be instantiated with some BHO class. We only have one BHO class in shipped code. There's a substitute BHO class used in the unit tests. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:34: template<class Key, class T, T nullValue> On 2016/02/01 15:50:42, sergei wrote: > We don't need `T nullValue`. See the unit tests. This class is easier to test when the argument is "class T" and not "class T*". https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:42: std::map<Key, T> idMap; On 2016/02/01 15:50:43, sergei wrote: > it would be better to call just "map". Acknowledged. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:47: std::mutex mutex; On 2016/02/01 15:50:42, sergei wrote: > it should has `mutable`. Done. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) On 2016/02/01 15:50:43, sergei wrote: > it would be better to call the arguments as "key" and "value". I don't want it completely generic. If this were a generic-use class, I'd agree with you. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) On 2016/02/01 15:50:41, sergei wrote: > What about constant references or more concrete interface? No need for that. A plain 'class T' is just fine. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; On 2016/02/01 15:50:43, sergei wrote: > `T` is not necessarily default constructible, it's better to use `emplace` or > `insert` here. How is default construction an issue here? 'p' is an argument; we already have an instance. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:73: auto it = idMap.find(id); On 2016/02/01 15:50:42, sergei wrote: > Can we do instead `return 1 == idMap.erase(id);`? Other than being written as a yoda condition, that would also be correct. I find that far less clear, though. The way it's written now, it's an obvious parallel with 'AddIfAbsent()' immediately above. If this were a performance-critical function, I'd consider the optimization. As it is, clarity predominates. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:83: * Returns a non-null pointer if a key of value 'id' is present. On 2016/02/01 15:50:42, sergei wrote: > This comment does not fit the template class. We don't know whether T is a > pointer or not. Acknowledged. I probably wrote this comment before I wrote the unit tests. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:86: T Locate(Key id) On 2016/02/01 15:50:42, sergei wrote: > I would rather prefer to pass T by reference to use it as a destination for the > copy of stored object and return `bool` to indicate whether it's found or not. No need. Putting return values in references is never a great pattern, but sometimes necessary. Here we don't need it. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:86: T Locate(Key id) On 2016/02/01 15:50:42, sergei wrote: > This method should be constant method. Done. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:90: return (it != idMap.end()) ? it->second : nullValue; On 2016/02/01 15:50:43, sergei wrote: > As far as I remember () are not needed here. Not necessary, but ternary expressions are always easier to read with parentheses around the condition (unless it's a single variable). https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:532: s_threadInstances[::GetCurrentThreadId()] = this; On 2016/02/01 15:50:43, sergei wrote: > Pay attention that we are overriding the value of `CPluginClass*` for > `::GetCurrentThreadId()`. I don't know what you mean here. > As it has been said in comments earlier, it looks logical to set the current > CPluginClass* from current UI thread. It's not logical; it's incorrect. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:67: On 2016/02/01 15:50:43, sergei wrote: > Why not to put it into anonymous namespace? OK. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:96: CurrentThreadMap threadMap; On 2016/02/01 15:50:43, sergei wrote: > Off topic: Still a global variable. When will we stop it?... The map from threads to BHO instances is inherently a singleton, and thus will always act much like a global. Just can't get around it. Global variables are not inherently bad. Global variables that could be scoped smaller are what's bad. With the state that 'CPluginClass' is in right now, combining BHO and UI responsibilities, we can't do incrementally better. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/PluginCl... src/plugin/PluginClass.cpp:251: throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map"); On 2016/02/01 15:50:43, sergei wrote: > That looks suspicious. It can happen that two instances are instantiated in one > thread Actually, that cannot happen. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... File test/plugin/InstancesTest.cpp (right): https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:20: typedef SyncMap<int, int, 0> SyncMapOne; On 2016/02/01 15:50:43, sergei wrote: > What does "One" mean? It means its the first 'SyncMap' for testing. If we need another, we can call it 'SyncMapTwo'.
I would propose to split this change into two commits, the first one contains SyncMap and the second one is for issue #3397, so in case of a trouble we can easily revert only that change. I think it would be better to discuss whether it should be implemented or not and how firstly in issue tracker, please find my comment there. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/03 17:17:04, Eric wrote: > On 2016/02/01 15:50:42, sergei wrote: > > It would be better to rename the file to SyncMap.h > > See https://codereview.adblockplus.org/29333107/. There are two classes related > to BHO instances to be defined here. In that case it would be better to have two files SyncMap.h and SyncSet.h because they contain quite generic classes. It seems it would be also good to move them into "shared". https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) On 2016/02/03 17:17:03, Eric wrote: > On 2016/02/01 15:50:43, sergei wrote: > > it would be better to call the arguments as "key" and "value". > > I don't want it completely generic. If this were a generic-use class, I'd agree > with you. I disagree here. If it's written as generic and it can be used with almost any type then it should have a proper interface. Otherwise it should not be generic. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; On 2016/02/03 17:17:04, Eric wrote: > On 2016/02/01 15:50:43, sergei wrote: > > `T` is not necessarily default constructible, it's better to use `emplace` or > > `insert` here. > > How is default construction an issue here? 'p' is an argument; we already have > an instance. `map<K, T>::operator[]` firstly creates T() and then calls assign operator with p as the argument. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:86: T Locate(Key id) On 2016/02/03 17:17:03, Eric wrote: > On 2016/02/01 15:50:42, sergei wrote: > > I would rather prefer to pass T by reference to use it as a destination for > the > > copy of stored object and return `bool` to indicate whether it's found or not. > > No need. Putting return values in references is never a great pattern, but > sometimes necessary. Here we don't need it. IMO having default `nullValue` here is a bad design. The main reason is that it's used here to infer that there is no entry with the passed key, but one can easily put `nullptr` into that container (and it's a normal case), so there is indeed an entry with that value but the caller will think that there is no such entry. Secondly, the caller should be able to obtain that nullValue from the container (class or instance), otherwise it looks like a magic value in the code. We use it merely in one place, so it seems that having `nullValue` requires too much efforts. As I said before we don't need it. I think it's not only an over-engineering it's rather a bad design. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:90: return (it != idMap.end()) ? it->second : nullValue; On 2016/02/03 17:17:04, Eric wrote: > On 2016/02/01 15:50:43, sergei wrote: > > As far as I remember () are not needed here. > > Not necessary, but ternary expressions are always easier to read with > parentheses around the condition (unless it's a single variable). That's very subjective. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... File test/plugin/InstancesTest.cpp (right): https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:20: typedef SyncMap<int, int, 0> SyncMapOne; On 2016/02/03 17:17:05, Eric wrote: > On 2016/02/01 15:50:43, sergei wrote: > > What does "One" mean? > > It means its the first 'SyncMap' for testing. If we need another, we can call it > 'SyncMapTwo'. Let's solve the problem of name collision when it really happens. Here it would be better to call it `IntSyncMap`. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:22: TEST(SyncMap, Instantiate) Where is the test "Destroy"? I'm not sure that we need such test. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:30: ASSERT_TRUE(s.AddIfAbsent(1, 11)); I still think that we should use EXPECT_ instead of ASSERT_. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instance... src/plugin/Instances.h:74: bool RemoveAndCheck(Key id) I think that only "Remove" would be better here. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/PluginCl... src/plugin/PluginClass.cpp:284: throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map"); I would also like to have asserts here and for RemoveAndCheck to be notified in time. https://codereview.adblockplus.org/29330403/diff/29335498/test/plugin/Instanc... File test/plugin/InstancesTest.cpp (right): https://codereview.adblockplus.org/29330403/diff/29335498/test/plugin/Instanc... test/plugin/InstancesTest.cpp:3: * Copyright (C) 2006-2015 Eyeo GmbH 2016
Patch set 4 fixes a copyright date. It also includes a rebase. On 2016/02/08 13:35:37, sergei wrote: > I would propose to split this change into two commits, the first one contains > SyncMap and the second one is for issue #3397, so in case of a trouble we can > easily revert only that change. We did not revert build-breaking changes involving ATL. I don't see any possibility that we'll be reverting any change here. If there's a problem, we'll add a new change that fixes it. That's how we've done everything else. > I think it would be better to discuss whether it should be implemented or not > and how firstly in issue tracker, please find my comment there. I've responded there. No change in plans for the present change set. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/08 13:35:35, sergei wrote: > In that case it would be better to have two files SyncMap.h and SyncSet.h > because they contain quite generic classes. It seems it would be also good to > move them into "shared". We're not writing Java. One header for two related containers for BHO instances is just fine. We will never need these containers in the engine, because they're especially written to be containers of BHO instances, and the engine does not have any BHO classes. They can stay right here in the plugin project. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) On 2016/02/08 13:35:35, sergei wrote: > If it's written as generic and it can be used with almost any > type then it should have a proper interface. Otherwise it should not be generic. Please tell that to 'std::basic_string', which ordinarily instantiates only with one of two types. 'basic_string' can _not_ "be used with almost any type". Just because you have a template doesn't mean you can stick anything at all in the argument. Perhaps the various iterations of "Concepts" that have been kicking around the C++ standards committee for years (at least the last three rounds) will come to fruition and there will be a decent way of specifying requirements on template arguments. Until then, you just declare template arguments without such specification. You haven't said anything new on this point for a couple rounds of comments. Just drop it. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; On 2016/02/08 13:35:35, sergei wrote: > `map<K, T>::operator[]` firstly creates T() and then calls assign operator with > p as the argument. No it doesn't. It constructs the value type in-place. Assignment is used nowhere in any variant of this function. If it were to require assignment, the container would only be available to types that were default-constructible. http://en.cppreference.com/w/cpp/container/map/operator_at https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:86: T Locate(Key id) On 2016/02/08 13:35:36, sergei wrote: > IMO having default `nullValue` here is a bad design. The alternative is a traits class, which would over-engineering for the present usage, although not for a general library function, which this is not. Some classes have a special value that means "nothing", other classes do not. It would be useful if the C++ standard library provided such a traits class that could be simply overloaded to specify such a property. No such standard property query, though, exists in <type_traits>. Lacking that, it's easier just to put it in a template argument. > The main reason is that it's used here to infer that there is no entry with the > passed key, but one can easily put `nullptr` into that container You can't put 'nullptr' into a container that's not instantiated with pointer class. Had I intended the container to only supported pointers I would have declared the template argument "class T*" and not "class T". > We use > it merely in one place, so it seems that having `nullValue` requires too much > efforts. We use it in the unit tests. Are you arguing that we should eliminate the unit tests so that it actually is only used in one place? https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:90: return (it != idMap.end()) ? it->second : nullValue; On 2016/02/08 13:35:36, sergei wrote: > That's very subjective. It should be manifestly obvious by now that your personal preferences differs from mine. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... File test/plugin/InstancesTest.cpp (right): https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:20: typedef SyncMap<int, int, 0> SyncMapOne; On 2016/02/08 13:35:36, sergei wrote: > Here it would be better to call it `IntSyncMap`. Bikeshedding. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:22: TEST(SyncMap, Instantiate) On 2016/02/08 13:35:36, sergei wrote: > Where is the test "Destroy"? > > I'm not sure that we need such test. This test also calls the destructor. https://codereview.adblockplus.org/29330403/diff/29332460/test/plugin/Instanc... test/plugin/InstancesTest.cpp:30: ASSERT_TRUE(s.AddIfAbsent(1, 11)); On 2016/02/08 13:35:36, sergei wrote: > I still think that we should use EXPECT_ instead of ASSERT_. 'EXPECT' is inferior here, and it isn't just personal preference. If 'AddIfAbsent' fails, then it's assured that the other two calls later will fail. That's two extra lines in the error log that provide no new information, but that the developer still has to understand. It's a bad idea. Rejected. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instance... src/plugin/Instances.h:74: bool RemoveAndCheck(Key id) On 2016/02/08 13:35:36, sergei wrote: > I think that only "Remove" would be better here. OK. I don't. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/PluginCl... src/plugin/PluginClass.cpp:284: throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map"); On 2016/02/08 13:35:37, sergei wrote: > I would also like to have asserts here and for RemoveAndCheck to be notified in > time. No. We have error handling already. Doubling it up is just code bloat. https://codereview.adblockplus.org/29330403/diff/29335498/test/plugin/Instanc... File test/plugin/InstancesTest.cpp (right): https://codereview.adblockplus.org/29330403/diff/29335498/test/plugin/Instanc... test/plugin/InstancesTest.cpp:3: * Copyright (C) 2006-2015 Eyeo GmbH On 2016/02/08 13:35:37, sergei wrote: > 2016 Done.
Overall I am fine with this after all. I am actually not able reproduce the case when OnWindowStateChanged is called from a different thread. It looks like it was a false alarm before. I think it is the case only on older IE versions. But even if I were able to reproduce it, there would still be a memory leak issue and we would be better off storing thread id in SetSite, as Eric mentioned anyway. I have just a few nits, which mostly have to do with renaming, but other then that I'd be fine with having this pushed. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) On 2016/02/08 18:45:29, Eric wrote: > On 2016/02/08 13:35:35, sergei wrote: > > If it's written as generic and it can be used with almost any > > type then it should have a proper interface. Otherwise it should not be > generic. > > Please tell that to 'std::basic_string', which ordinarily instantiates only with > one of two types. I think if we are not implementing a proper interface there isn't any reason then to have both this and CurrentThreadMap later. We can just have one type instead. But if we are treating this as a base class, hinting other types as well, we can just implement it properly. That being said, I don't feel this is very important and I would be fine committing this as is too. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instance... src/plugin/Instances.h:74: bool RemoveAndCheck(Key id) On 2016/02/08 18:45:30, Eric wrote: > On 2016/02/08 13:35:36, sergei wrote: > > I think that only "Remove" would be better here. > > OK. I don't. What about RemoveIfPresent? Just for the consistency with the Add method. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/PluginCl... src/plugin/PluginClass.cpp:284: throw std::logic_error("CPluginClass::SetSite - May not overwrite existing entry in thread map"); On 2016/02/08 18:45:31, Eric wrote: > On 2016/02/08 13:35:37, sergei wrote: > > I would also like to have asserts here and for RemoveAndCheck to be notified > in > > time. > > No. We have error handling already. Doubling it up is just code bloat. I don't think we need asserts here as well. https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instance... src/plugin/Instances.h:1: /* Nit: If I understand correctly this file will have both SyncMap and SyncSet defined. I think it would be better to have them in separate files, each file named after the class. Instances.h isn't really descriptive, IMO. Alternatively this file can be SyncCollections.h https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1605: CPluginTab* tab = GetTabCurrentThread(); Nit: I think the whitespace change here is accidental? https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... src/plugin/PluginClass.h:89: static CPluginTab* GetTabCurrentThread(); Nit: the name kind of implies the return value would be a thread, IMO. How about GetTabForCurrentThread?
Patch set 5 renames a couple of symbols. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:54: bool AddIfAbsent(Key id, T p) On 2016/02/10 10:58:47, Oleksandr wrote: > That being said, I don't feel this is very important and I would be fine > committing this as is too. Good. > On 2016/02/08 18:45:29, Eric wrote: > I think if we are not implementing a proper interface there isn't any reason > then to have both this and CurrentThreadMap later. Unit testing is the reason to split them up. You can't test all the basic cases if you only use the current thread ID as a test parameter. https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29335498/src/plugin/Instance... src/plugin/Instances.h:74: bool RemoveAndCheck(Key id) On 2016/02/10 10:58:48, Oleksandr wrote: > What about RemoveIfPresent? Just for the consistency with the Add method. OK. Changed. In retrospect, these might better be 'CheckAbsenceThenAdd' and 'CheckPresenceThenRemove', but that's a little wordy. https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/10 10:58:49, Oleksandr wrote: > Nit: If I understand correctly this file will have both SyncMap and SyncSet > defined. I think it would be better to have them in separate files, each file > named after the class. Instances.h isn't really descriptive, IMO. Alternatively > this file can be SyncCollections.h Synchronization isn't fundamental to what these classes do, even though it is necessary to have it so that they function correctly. Their function as containers is rather more fundamental. The details of these containers are quite specific to collections of BHO instances within IE, even if a rather non-specific way they could possibly be used for something we have no idea of yet. How about "InstanceContainer.h" or "BHOInstanceContainer.h" or "BHOContainer.h"? https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1605: CPluginTab* tab = GetTabCurrentThread(); Yes. Fixed. https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/PluginCl... src/plugin/PluginClass.h:89: static CPluginTab* GetTabCurrentThread(); On 2016/02/10 10:58:49, Oleksandr wrote: > Nit: the name kind of implies the return value would be a thread, IMO. How about > GetTabForCurrentThread? Good idea. OK.
LGTM
Although the code might work I cannot approve it because of the style. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/08 18:45:29, Eric wrote: > On 2016/02/08 13:35:35, sergei wrote: > > In that case it would be better to have two files SyncMap.h and SyncSet.h > > because they contain quite generic classes. It seems it would be also good to > > move them into "shared". > > We're not writing Java. One header for two related containers for BHO instances > is just fine. So far there is only one container which is named as a generic container. It's just a good practice to name files according to the content of files. Even if there are some generic container like SyncMap and SyncSet the file should be named SyncContainers. "Instances" and SyncMap have nothing common. > > We will never need these containers in the engine, because they're especially > written to be containers of BHO instances, and the engine does not have any BHO > classes. They can stay right here in the plugin project. If they are written only to store pointers to BHO instances and use thread ID as a key then they should not be generic. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:21: * A base class for a synchronized map from threads to BHO instances. On 2016/02/03 17:17:03, Eric wrote: > On 2016/02/01 15:50:41, sergei wrote: > > Either the comment or class should not be so thread-pointer specific. Either > > it's a template SyncMap or it's a concrete class without any templates. > > It's a template class to be instantiated with some BHO class. We only have one > BHO class in shipped code. There's a substitute BHO class used in the unit > tests. I think that we should not overcomplicate the code only to have an ability to use `int` instead of a particular class or just any auxiliary class in unit tests. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; On 2016/02/08 18:45:30, Eric wrote: > On 2016/02/08 13:35:35, sergei wrote: > > `map<K, T>::operator[]` firstly creates T() and then calls assign operator > with > > p as the argument. > > No it doesn't. It constructs the value type in-place. Assignment is used nowhere > in any variant of this function. If it were to require assignment, the container > would only be available to types that were default-constructible. > > http://en.cppreference.com/w/cpp/container/map/operator_at > So, it describes exactly what I had told. One can even try the code there, just try it #include <map> class X { public: X(int i){} }; int main() { std::map<int, X> m; m[10] = X(10); return 0; } https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/05/19 17:23:58, Eric wrote: > On 2016/02/10 10:58:49, Oleksandr wrote: > > Nit: If I understand correctly this file will have both SyncMap and SyncSet > > defined. I think it would be better to have them in separate files, each file > > named after the class. Instances.h isn't really descriptive, IMO. > Alternatively > > this file can be SyncCollections.h > > Synchronization isn't fundamental to what these classes do, even though it is > necessary to have it so that they function correctly. Their function as > containers is rather more fundamental. The details of these containers are quite > specific to collections of BHO instances within IE, even if a rather > non-specific way they could possibly be used for something we have no idea of > yet. > > How about "InstanceContainer.h" or "BHOInstanceContainer.h" or "BHOContainer.h"? I like "BHOContainer.h" but the classes' names should reflect it, they should not be so generic like SyncMap. https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instance... src/plugin/Instances.h:25: * A base class for a synchronized map from threads to BHO instances. This class is not designed to be a base class because there is no virtual destructor. https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instance... src/plugin/Instances.h:25: * A base class for a synchronized map from threads to BHO instances. As I told already there is no reason to have such class so generic if it is only for <threadID, BHOInstance*>. The interface of generic SyncMap<Key, Value> should be different. https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instance... src/plugin/Instances.h:37: */ We can save a lot of energy if move the implementation of this class into `CurrentThreadMap` and move `CurrentThreadMap` into this file. We will not need the comments, suspicious inheritance and some questions will be eliminated automatically.
New patch set. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:1: /* Both points addressed elsewhere. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:21: * A base class for a synchronized map from threads to BHO instances. On 2016/05/23 13:14:48, sergei wrote: > I think that we should not overcomplicate the code only to have an ability to > use `int` instead of a particular class or just any auxiliary class in unit > tests. This "complication" is part of the environment. We can't have unit tests with BHO instance because we don't run unit tests under IE. If we have units tests, we have some sort of mock class regardless. https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; I've rewritten the function to use insert(). The bigger problem with this function was a performance detail, since 'operator[]' implicitly calls the equivalent of 'find()'. https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29336109/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/05/23 13:14:48, sergei wrote: > I like "BHOContainer.h" but the classes' names should reflect it, they should > not be so generic like SyncMap. This file also appears in https://codereview.adblockplus.org/29333107/. At this point I'd rather just deal with its name then. As for class names, once we get to a point where this is the last remaining issue, we can revisit it. But honestly, this all seems like so much bikeshedding. This file is included in exactly one source file and it's classes (this plus the one in another review) are used exactly once. https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instance... src/plugin/Instances.h:25: * A base class for a synchronized map from threads to BHO instances. On 2016/05/23 13:14:49, sergei wrote: > As I told already there is no reason to have such class so generic if it is only > for <threadID, BHOInstance*>. The interface of generic SyncMap<Key, Value> > should be different. And as *I* said before, it is *not* only for instance pointers. Let me be very clear. (1) Use in the plugin for instance pointers (2) Use in unit tests for higher code quality. If you would like to argue that unit tests are unimportant and should be eliminated, I will listen to that argument. https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instance... src/plugin/Instances.h:25: * A base class for a synchronized map from threads to BHO instances. On 2016/05/23 13:14:49, sergei wrote: > This class is not designed to be a base class because there is no virtual > destructor. The rule is that you need a virtual destructor in a polymorphic base class, not any base class of any type. This class is not designed to be polymorphic; for example, it has no virtual functions. This class is separated out for testability. Design for testability is, at this point, a widely accepted best practice. I've added a comment in a new patch set, because evidently this all was not sufficiently clear. https://codereview.adblockplus.org/29330403/diff/29342817/src/plugin/Instance... src/plugin/Instances.h:37: */ On 2016/05/23 13:14:49, sergei wrote: > We can save a lot of energy if move the implementation of this class into > `CurrentThreadMap` and move `CurrentThreadMap` into this file. We will not need > the comments, suspicious inheritance and some questions will be eliminated > automatically. Everything in this source file can be easily unit tested. Anything using current thread cannot be easily tested. Furthermore, this is a header-only file containing only template definitions. Defining 'CurrentThreadMap' here would require an extra source file for just a few lines of code. It's better to leave that function where it is, in PluginClass.cpp, where it's defined at its point of use.
LGTM https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29332460/src/plugin/Instance... src/plugin/Instances.h:62: idMap[id] = p; On 2016/07/17 16:04:43, Eric wrote: > I've rewritten the function to use insert(). The bigger problem with this > function was a performance detail, since 'operator[]' implicitly calls the > equivalent of 'find()'. JIC, insert has the same complexity. https://codereview.adblockplus.org/29330403/diff/29347801/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29347801/src/plugin/Instance... src/plugin/Instances.h:65: auto it = idMap.insert(std::make_pair(id,p)); missing space after comma https://codereview.adblockplus.org/29330403/diff/29347801/src/plugin/Instance... src/plugin/Instances.h:68: // which means there was no key of value 'id' already present. I think we don't need this comment.
Fixed the missing space before commit. https://codereview.adblockplus.org/29330403/diff/29347801/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29330403/diff/29347801/src/plugin/Instance... src/plugin/Instances.h:65: auto it = idMap.insert(std::make_pair(id,p)); On 2016/07/19 07:56:09, sergei wrote: > missing space after comma Fixed. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
