Issue 29333107: Issue #1652, #3456 - Replace 's_asyncWebBrowser2' with thread-safe facilities
DescriptionIssue #1652, #3456 - Replace 's_asyncWebBrowser2' with thread-safe facilities
Remove 's_asyncWebBrowser2' and 'GetAsyncBrowser()'. Replace them with a new
class 'AnyCurrentSiteBrowser', which encapsulates an 'IWebBrowser2*' site
pointer and ensures that it remains a current site during its life span.
Update all the functions that had used 'GetAsyncBrowser()'.
Remove 's_instances'. Replace it with a new class 'SyncSet' with its own
mutex. A separate mutex is required to properly implement
'AnyCurrentSiteBrowser'. Rewrite explicit loops through 's_instances' with the
functional member 'LinearSearch()'.
Clarify the code in 'ShowStatusBar()', flattening its control flow and
improving its debug messages.
In 'FirstRunThread()', add exception handler around body. Fix missing call to
'CoUninitialize()'. Fix missing test for null browser.
Add function 'NavigateNewTab()', extracting duplicate code that had appeared
in three different places.
Change 'IsStatusBarEnabled()' to anonymous namespace. Renamed it to be more
descriptive.
Patch Set 1 : #
Total comments: 41
Patch Set 2 : address comments #
Total comments: 9
Patch Set 3 : rebase only #
MessagesTotal messages: 10
It'a good idea with 'AnyCurrentSiteBrowser', however it would be better to split the changes into several commits. They are quite independent and enough big. Don't worry, they seem pretty straightforward, so I guess we can quickly approve them. I would like to have at least the following in separate commits: > Clarify the code in 'ShowStatusBar()', flattening its control flow and improving its debug messages. > In 'FirstRunThread()', add exception handler around body. Fix missing call to 'CoUninitialize()'. Fix missing test for null browser. > Add function 'NavigateNewTab()', extracting duplicate code that had appeared in three different places. > Change 'IsStatusBarEnabled()' to anonymous namespace. Renamed it to be more descriptive. Removing of 's_asyncWebBrowser2' and 'GetAsyncBrowser()' can be also in a another commit after "Remove 's_instances'...". https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:25: * Template class for a synchronized set of BHO instances Do you mind to change the comment a bit? If it's a class for a set of BHO instances then it should not be a template class. However it can be useful to know that it's used for example to store pointers to instances of BHO and I don't mind to have it as a template class. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:41: std::mutex mutex; I guess it should be `mutable` because it can be used from constant methods, see below the notes regarding which methods can be constant. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:52: bool InsertIfAbsent(T p) it would be better to have instead methods which accept `const T&` and `T&&`, but only `const T&` is enough. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:72: bool EraseWithCheck(T p) The argument should be `const T&`. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) Should it be a constant method? https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) I would like to have a constant reference to T as the argument of the predicate, thus `std::function<bool(const T&)>`. In the current use we are working with the pointers and the copying of the value does not hurt but it would be wise to avoid copying here. If we change the method to a constant then we will have to deal with the const references. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) it would be also good to pass the predicate as a constant reference to the function. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:95: for (auto it : set) JIC, `it` does not refer to an iterator here, I would like to have a different name, for example `value`. In general it's already unwrapped iterator, thus a reference to a value, but this particular construction creates a copy of the value, to avoid it we should add `&`. In summary I would write it as `for (const auto& value : set)`. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:108: return set.empty(); SentryType sentry(mutex); is missed here https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:158: throw std::logic_error("Instance without site found in set of instances with site"); "... where site is a browser (IWebBrowser2) and instance refers to an instance of CPluginClass". Otherwise this message is completely unclear. Image that you encountered it in a log. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:298: if (!instanceSet.InsertIfAbsent(this)) I would like to have `assert`s in addition to throwing of exceptions here and below, otherwise the exceptions are silently eaten and we won't know about a trouble. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1136: AnyCurrentSiteBrowser browser; Why can we not use `m_webBrowser2` here? When can it be called not between SetSite(not nullptr) and SetSite(nullptr)? https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.h:147: * Synchronizes SetSite() with references to our site pointer. I would like to have here a little bit extended comment that it can be used, for example, to avoid any wrong usage of injected objects beyond the time span between SetSite(not nullptr) and SetSite(nullptr). For example, AnyCurrentSiteBrowser helps to achieve it.
On 2016/01/04 15:34:32, sergei wrote: > It'a good idea with 'AnyCurrentSiteBrowser', however it would be better to split > the changes into several commits. They are quite independent and enough big. > Don't worry, they seem pretty straightforward, so I guess we can quickly approve > them. I am rejecting your request to break these up. It's nothing but bureaucratic busywork. Sometimes there are substantive reasons to do so, but you haven't made any arguments about why it should be broken up other than your personal preference. Furthermore, I really don't believe they'll be quickly approved. To be blunt, that seems completely disingenuous. I have 11 reviews out where my most recent patch set is two weeks old or more, and in another two days there will be four more of them, and half of them will be over three weeks with no action by then. I might have some sympathy with that if I didn't have more than a dozen open review out with no response https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:25: * Template class for a synchronized set of BHO instances On 2016/01/04 15:34:31, sergei wrote: > Do you mind to change the comment a bit? I don't see a need, given what you've said so far. > If it's a class for a set of BHO instances then it should not be a template > class. (1) It's a template so that it can be instantiated for unit tests. It's basically impossible to instantiate 'CPluginClass' outside the whole plugin. This is a completely sufficient reason to use a template declaration. (2) We only have one class that acts as a BHO. This class could be used for any BHO class. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:41: std::mutex mutex; On 2016/01/04 15:34:31, sergei wrote: > I guess it should be `mutable` because it can be used from constant methods, see > below the notes regarding which methods can be constant. Done. The only 'const' method, though, is 'Empty()'. It's conceptually correct, though, to declare this member as 'mutable'. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:52: bool InsertIfAbsent(T p) On 2016/01/04 15:34:30, sergei wrote: > it would be better to have instead methods which accept `const T&` and `T&&`, > but only `const T&` is enough. If this were generic library code, we'd want declarations for all of these. As it is, this is *not* library code, and a simple declaration is adequate. There's only one declaration in the present code. In the plugin, 'T' is instantiated with a pointer, which you want to pass by value. You don't want a reference here at all. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:72: bool EraseWithCheck(T p) On 2016/01/04 15:34:30, sergei wrote: > The argument should be `const T&`. See above. Pass by value is desirable. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) On 2016/01/04 15:34:30, sergei wrote: > I would like to have a constant reference to T as the argument of the predicate, > thus `std::function<bool(const T&)>`. We're passing 'T' by value everywhere. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) On 2016/01/04 15:34:30, sergei wrote: > Should it be a constant method? No. If we wanted a second, 'const'-aware definition, then you could do that. We only need one, and it needs to be non-constant. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) On 2016/01/04 15:34:31, sergei wrote: > it would be also good to pass the predicate as a constant reference to the > function. It's only ever used with lambda functions, so an lvalue reference need not even work. This is a place where we can let the compiler do the right thing. No need to constrain it with excessive declarations. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:95: for (auto it : set) On 2016/01/04 15:34:30, sergei wrote: > JIC, `it` does not refer to an iterator here, I would like to have a different > name, for example `value`. OK. > In summary I would write it as `for (const auto& value : set)`. Again, there's no need to overconstrain the compiler. They're rather good at picking efficient type instantiations these days. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:108: return set.empty(); On 2016/01/04 15:34:30, sergei wrote: > SentryType sentry(mutex); > is missed here This function is only instantiated in the unit tests, but I added the statement anyway. It doesn't hurt. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:158: throw std::logic_error("Instance without site found in set of instances with site"); On 2016/01/04 15:34:31, sergei wrote: > "... where site is a browser (IWebBrowser2) and instance refers to an instance > of CPluginClass". Otherwise this message is completely unclear. Image that you > encountered it in a log. When I encounter such messages in a log, I immediately use 'grep' to search the source code for the occurrence of the statement. The exact text doesn't need to be extensive. More importantly, though, it's a 'logic_error', which means that your code has a defect if it ever throws at all. If you find such a message in a log, you've got bigger problems. I have clarified the message, not to talk about specific classes, but to more clearly explain the invariant that is violated if reached here. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:298: if (!instanceSet.InsertIfAbsent(this)) On 2016/01/04 15:34:31, sergei wrote: > I would like to have `assert`s in addition to throwing of exceptions here and > below, otherwise the exceptions are silently eaten and we won't know about a > trouble. 'assert' is a C mechanism, and it's only active during debug. This is very consciously a 'throw' here. While we currently have no capability of logging in Release builds, this is the kind of exception that we want to log to the Windows Error Reporting facility. We don't have code for that, yet. If you want to catch them, set up Visual Studio to do so. In Debug/Windows/Exceptions Settings, add "std::logic_error" to the list called "Break When Thrown". https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.h:147: * Synchronizes SetSite() with references to our site pointer. On 2016/01/04 15:34:31, sergei wrote: > I would like to have here a little bit extended comment There's nothing I could add here that's not fully explained in the already-extensive comments for 'AnyCurrentSiteBrowser'. The term "synchronizes with" is jargon from the C++ language standard. I'm assuming a reader knows that. With that in mind, the comment completely explains what this member variable does. You seem to be looking for "how it's used" comments, but those are elsewhere.
Patch set 2 addresses Sergei's comments that I accepted.
I'm waiting for the opinion of others
On 2016/01/11 10:37:29, sergei wrote: > I'm waiting for the opinion of others If you have an objection to the current code, say so. I've addressed all the ones you've made so far. If you think my responses to your objections are wrong, then say so. If you can't make an argument about why the code doesn't work, then say so.
On 2016/01/29 14:21:39, Eric wrote: > On 2016/01/11 10:37:29, sergei wrote: > > I'm waiting for the opinion of others > > If you have an objection to the current code, say so. I've addressed all the > ones you've made so far. If you think my responses to your objections are wrong, > then say so. If you can't make an argument about why the code doesn't work, then > say so. Firstly I disagree with rejection to split the changes into several commits. There are self-contained changes and it would be better to have them in separate commits. As well as I'm personally ready to approve some of those changes but not all. I have answered with what I disagree in comments. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:25: * Template class for a synchronized set of BHO instances On 2016/01/04 17:49:20, Eric wrote: > On 2016/01/04 15:34:31, sergei wrote: > > Do you mind to change the comment a bit? > > I don't see a need, given what you've said so far. I disagree that the description of generic template class SyncSet can say that it's "set of BHO instances". > > > If it's a class for a set of BHO instances then it should not be a template > > class. > > (1) It's a template so that it can be instantiated for unit tests. It's > basically impossible to instantiate 'CPluginClass' outside the whole plugin. > This is a completely sufficient reason to use a template declaration. I'm pretty sure that it's possible to instantiate `CPluginClass` in tests. I have just done it: ATL::CComObject<CPluginClass>* impl; ATL::CComObject<CPluginClass>::CreateInstance(&impl); > > (2) We only have one class that acts as a BHO. This class could be used for any > BHO class. > https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:52: bool InsertIfAbsent(T p) On 2016/01/04 17:49:20, Eric wrote: > On 2016/01/04 15:34:30, sergei wrote: > > it would be better to have instead methods which accept `const T&` and `T&&`, > > but only `const T&` is enough. > > If this were generic library code, we'd want declarations for all of these. As > it is, this is *not* library code, and a simple declaration is adequate. > > There's only one declaration in the present code. In the plugin, 'T' is > instantiated with a pointer, which you want to pass by value. You don't want a > reference here at all. I disagree with that attitude. I think it should be either BHOSyncSet without any templates or there should be another interface. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) On 2016/01/04 17:49:20, Eric wrote: > On 2016/01/04 15:34:30, sergei wrote: > > Should it be a constant method? > > No. > > If we wanted a second, 'const'-aware definition, then you could do that. We only > need one, and it needs to be non-constant. This method does not change the main sate of the class, so it should be constant. Specifically to support it std::mutex has `mutable`. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) On 2016/01/04 17:49:20, Eric wrote: > On 2016/01/04 15:34:31, sergei wrote: > > it would be also good to pass the predicate as a constant reference to the > > function. > > It's only ever used with lambda functions, so an lvalue reference need not even > work. This is a place where we can let the compiler do the right thing. No need > to constrain it with excessive declarations. It's just a dirty code, isn't it clear? https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:298: if (!instanceSet.InsertIfAbsent(this)) On 2016/01/04 17:49:20, Eric wrote: > On 2016/01/04 15:34:31, sergei wrote: > > I would like to have `assert`s in addition to throwing of exceptions here and > > below, otherwise the exceptions are silently eaten and we won't know about a > > trouble. > > 'assert' is a C mechanism, and it's only active during debug. `assert` is also C++ mechanism. Yes, it's active only during debugging because its main aim is to notify the developer about an issue. I'm not suggesting to replace throwing, but add `assert` in addition. > > This is very consciously a 'throw' here. While we currently have no capability > of logging in Release builds, this is the kind of exception that we want to log > to the Windows Error Reporting facility. We don't have code for that, yet. > > If you want to catch them, set up Visual Studio to do so. In > Debug/Windows/Exceptions Settings, add "std::logic_error" to the list called > "Break When Thrown". That's very inconvenient because of too many false positives. I could use a special Visual Studio profile only for adblockplusie project, but it's really not friendly for anybody else. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1136: AnyCurrentSiteBrowser browser; On 2016/01/04 15:34:31, sergei wrote: > Why can we not use `m_webBrowser2` here? When can it be called not between > SetSite(not nullptr) and SetSite(nullptr)? Not answered.
None of the comments require code changes. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:25: * Template class for a synchronized set of BHO instances Your objection here is that you don't like the template. Since you haven't identified a defect in the code, I'll assume you haven't found one. You're arguing style. I find it better to use a template class because it promotes better testing. You don't find value in that. I reject your request to rewrite this without a template. I consider it a step backwards. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:52: bool InsertIfAbsent(T p) You haven't identified a defect here. You have not made any argument about how using a reference would improve the code. I have made an argument about how using a reference would diminish the code. You have not responded to that point. Change request rejected. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:92: T LinearSearch(std::function<bool(T)> f) The whole discussion of 'const' declaration here has devolved to bikeshedding. It does not reveal any defect in the code, merely a way that this class is not as general as a library class would be. On 2016/01/29 15:28:55, sergei wrote: > It's just a dirty code, isn't it clear? No. You have not sufficiently explained why it's dirty. We could declare the argument 'f' to have constant effect, which would allow this function to have constant effect. But then we'd need this function in addition. What you're recommending just feels like code bloat. On 2016/01/29 15:28:55, sergei wrote: > This method does not change the main sate of the class, so it should be > constant. The argument 'f' does not have constant effect. Therefore it can change the state of the members of the container. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:298: if (!instanceSet.InsertIfAbsent(this)) You're advocating code bloat for zero-to-minimal benefit. The presence of this check here in 'SetSite()' is purely defensive. IE creates a new thread for each BHO instance. Request add 'assert' here rejected. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1136: AnyCurrentSiteBrowser browser; On 2016/01/29 15:28:56, sergei wrote: > Not answered. Sorry. Must have missed it earlier. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/PluginCl... src/plugin/PluginClass.cpp:1136: AnyCurrentSiteBrowser browser; On 2016/01/04 15:34:31, sergei wrote: > Why can we not use `m_webBrowser2` here? This function can be called from a worker thread which is not associated with any BHO at all. > When can it be called not between > SetSite(not nullptr) and SetSite(nullptr)? These worker threads never call 'SetSite()' at all. (And it's never the case that any single thread is ever reused for a second BHO instance, although that's not particularly relevant here.) The race condition is that we have events registered in such a way that it's possible that a call appear here in one thread (a worker thread) where another thread has already called 'SetSite(nullptr)' on the thread associated with this BHO instance. Hence it's possible for 'm_webBrowser2' to be null here. What you're observing here is a consequence of some really awful (and quite old) implementation decisions about 'CPluginClass'. It is handling both BHO instances as well as UI events. They really should never have been combined into a single class in the first place. Regardless of the past, if the UI event handlers were in a separate class, then 'AnyCurrentSiteBrowser' is how such a class would get a browser pointer to call 'Navigate()' on.
No comments regarding the AnyCurrentSiteBrowser - that part is fine by me. The comments I do have, I guess will be considered "bikeshedding", but in this case I think these are important. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:25: * Template class for a synchronized set of BHO instances On 2016/01/29 16:16:12, Eric wrote: > Your objection here is that you don't like the template. Since you haven't > identified a defect in the code, I'll assume you haven't found one. > > You're arguing style. I find it better to use a template class because it > promotes better testing. You don't find value in that. > > I reject your request to rewrite this without a template. I consider it a step > backwards. > I think Sergei was merely talking about the comment here, not the implementation. Why say it is a class for BHO instances when it can be so much more. We are using it here as well https://codereview.adblockplus.org/29330403/ for example. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:52: bool InsertIfAbsent(T p) On 2016/01/29 16:16:12, Eric wrote: > You haven't identified a defect here. > > You have not made any argument about how using a reference would improve the > code. > > I have made an argument about how using a reference would diminish the code. You > have not responded to that point. > > Change request rejected. I don't have a strong opinion on this, but I agree that we should treat this class as a generic one, not BHO specific one. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/Instance... src/plugin/Instances.h:1: /* Nit: This file is also being added in https://codereview.adblockplus.org/29330403/. I'm guessing this review builds on top of that one? https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.cpp:396: bool IsStatusBarDisabledInRegistry() I see no reason why IsStatusBarEnabled is refactored in this change set. It is unrelated, IMO. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.cpp:464: if (!isVisible) Same here. Why is this done in the change set with the message "Replace 's_asyncWebBrowser2' with thread-safe facilities"? I don't think this is related at all. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.cpp:975: NavigateNewTab(browser, FirstRunPageFileUrl()); I agree this looks better with NavigateNewTab separated, but this is not relevant to the present change at all. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... File src/plugin/PluginClass.h (left): https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.h:139: bool IsStatusBarEnabled(); Nit: Unrelated change.
On 2016/02/01 18:03:15, Oleksandr wrote: > No comments regarding the AnyCurrentSiteBrowser - that part is fine by me. The > comments I do have, I guess will be considered "bikeshedding", but in this case > I think these are important. Bikeshedding is not raising a small concern the first time. It's spending too much time on a small concern. Your main concern seems to be with apparently-unrelated changes. This change set has two kinds of changes: implementing class 'AnyCurrentSiteBrowser' and using it. When I was working on the second of these two, I rewrote some code at the point of use to make sure that it was being used correctly, since that was rather more difficult with the old code. These changes could be split off, but it would be a fair bit of work to make two change sets, test the new one to make sure it neither breaks the build nor is defective, and all for committing two change sets separately in a history that we are very unlikely to revisit. We did not revisit changes that broke the build (two of them, both involving ATL), so it seems of low value to split these changes up. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:25: * Template class for a synchronized set of BHO instances On 2016/02/01 18:03:13, Oleksandr wrote: > I think Sergei was merely talking about the comment here, not the > implementation. I don't believe so. He has said more that once that he would prefer it not be a template class. If it's not a template class, you get inferior unit testing. > Why say it is a class for BHO instances when it can be so much > more. Because we're never going to use it for anything more. This class is specifically targeted at the behavior of IE and the way it calls 'SetSite()' on BHO instances. Its behaviors aren't so weird that it couldn't be used elsewhere, but it was written to deal with BHO semantics, and it's the only way we'll use it. I see no downside in documenting the class with a less-than-fully general comment that states very specifically the environment the class is written for. > We are using it here as well https://codereview.adblockplus.org/29330403/ > for example. In fact we are _not_ using it there. The present class is 'SyncSet'. The one there is 'SyncMap'. The use different underlying containers. What they do have in common is similar behaviors specialized to be containers of BHO objects and managed through 'SetSite()' as called by IE. https://codereview.adblockplus.org/29333107/diff/29333114/src/plugin/Instance... src/plugin/Instances.h:52: bool InsertIfAbsent(T p) On 2016/02/01 18:03:13, Oleksandr wrote: > I don't have a strong opinion on this, but I agree that we should treat this > class as a generic one, not BHO specific one. Answered above. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/Instances.h File src/plugin/Instances.h (right): https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/Instance... src/plugin/Instances.h:1: /* On 2016/02/01 18:03:14, Oleksandr wrote: > Nit: This file is also being added in > https://codereview.adblockplus.org/29330403/. I'm guessing this review builds on > top of that one? No. I made sure they were not interdependent. The idea, though, is that both 'SyncSet' and 'SyncMap' end up in the same source file, since they are related classes that each deal with containers of BHO instances. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.cpp:396: bool IsStatusBarDisabledInRegistry() On 2016/02/01 18:03:14, Oleksandr wrote: > I see no reason why IsStatusBarEnabled is refactored in this change set. It is > unrelated, IMO. It's not entirely unrelated. These changes tag along with the changes of type of the various browser variables from 'IWebBrowser2' or 'CComPtr<IWebBrowser2' to 'AnyCurrentSiteBrowser'. Strictly speaking, they could go into a separate change set, but it would be a "noissue" commit. I don't see enough value in creating two change sets here to go through the effort of splitting the change set up. It might be worth changing the commit message to reflect that there are "noissue" changes tagging along here to related parts of the code. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.cpp:464: if (!isVisible) On 2016/02/01 18:03:14, Oleksandr wrote: > Same here. Why is this done in the change set with the message "Replace > 's_asyncWebBrowser2' with thread-safe facilities"? I don't think this is related > at all. See above. https://codereview.adblockplus.org/29333107/diff/29333153/src/plugin/PluginCl... src/plugin/PluginClass.cpp:975: NavigateNewTab(browser, FirstRunPageFileUrl()); On 2016/02/01 18:03:14, Oleksandr wrote: > I agree this looks better with NavigateNewTab separated, but this is not > relevant to the present change at all. When I went through and changed types to use 'AnyCurrentSiteBrowser', I went ahead and improved the code in its vicinity. As I've said elsewhere, it could be split into a "noissue" commit, but I don't see a lot of value in that at this point. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
