Issue 29332848: Issue #3432 - Manage COM events with a resource class
DescriptionIssue #3432 - Manage COM events with a resource class
Add 'AtlEventRegistration', a resource class that registers for events in its
constructor and unregisters in it destructor. Add 'AtlEventRegistrationHolder'
which hold an event resource, exposing 'Start()' and 'Stop()' methods instead.
Add 'CPluginClass::browserEvents' to encapsulate these events and remove the
superseded flag 'm_isAdvised' and member function 'Unadvise()'. Call 'Start()'
and 'Stop()' once each in 'SetSite()'.
Add 'CPluginClass::detachedInitializationFailed' to capture the state of a
possible failure. Added a guards to check this flag at the beginning of each
event handler, having the same effect as if events were not being received.
Remove call to disable events when the initialization does fail.
Removed 'CPluginClass::OnOnQuit' entirely. It's only code was to deregister
events. Deregistering now works reliably in both 'SetSite()' and in the
destructor, so there's no need for special handling.
Patch Set 1 : #Patch Set 2 : rebase #
Total comments: 34
Patch Set 3 : rebase, address comments #
Total comments: 4
Patch Set 4 : rebase only #MessagesTotal messages: 10
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.cpp:261: // Start event last, after everything is ready to go This comment is confusing because not everything is ready yet, just a couple of lines above we are starting some asynchronous initialization. It's clear that before binding with event listeners we need to prepare other members, but apparently not everything is ready to go yet. IMO, it would be better without this comment. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.cpp:454: if (detachedInitializationFailed) return; it would be better to use some method `bool IsInitialized() const`, it's a bit confusing that we are checking only result of "detached" initialization. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.cpp:454: if (detachedInitializationFailed) return; Basically all these checks in each method are indicating that something is wrong with the design. It would be better to bind event listeners only after finishing with the prerequisites. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:54: class AtlEventRegistration I doubt that we need two classes here, AtlEventRegistrationHolder and AtlEventRegistration, only one would be completely enough. The code is correct but it's overengineering. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:74: throw std::runtime_error("DispEventAdvise() failed"); I would prefer to make private constructor and have a static creator method (e.g. `static std::unique_ptr<AtlEventRegistration<Sink, Source>> Create(Sink* consumer, Source* producer);`) instead of AtlEventRegistrationHolder and such usage of exceptions. In addition we can have a public typedef for `std::unique_ptr<AtlEventRegistration<Sink, Source>>`. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:115: : impl(nullptr) This line as well as the empty constructor and destructor are not needed. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:129: impl = nullptr; This line is not needed. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:137: impl = nullptr; it's correct, but I find it better to use impl.reset(); https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:241: * Event manager for events coming from our site object. Why do we need this comment? https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:244: bool detachedInitializationFailed; Since it's accessed from different threads, there should be a proper access methods, e.g. it can be std::atomic<bool> or protected by some synchronization primitive if we actually need this member. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:244: bool detachedInitializationFailed; The member naming is not consistent with other members because "m_" prefix is missed.
Patch set 3 includes rebase and addresses Sergei's comments. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.cpp:261: // Start event last, after everything is ready to go On 2016/01/06 08:41:57, sergei wrote: > This comment is confusing because not everything is ready yet Reworded. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.cpp:454: if (detachedInitializationFailed) return; On 2016/01/06 08:41:57, sergei wrote: > Basically all these checks in each method are indicating that something is wrong > with the design. Something is definitely wrong with the design. It's not something, however, that's going to be quick to change. The present change set is only incremental progress. The biggest single problem is that 'CPluginClass' is too much of a god class. The current detached initialization thread is working with what should be several different classes. Synchronization for the completion of initialization should be isolated into these classes. Until that's (partially) accomplished, the real issues won't be clear. In addition, the lack of transactional (atomic) registration already causes race conditions we should be dealing with. See #3433 for that aspect. > It would be better to bind event listeners only after finishing > with the prerequisites. That's something that may not be strictly possible. (And it might not even be desirable.) Regardless, it's premature to talk about it at any great length right now. The code is in too poor a shape to make that a productive conversation yet. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.cpp:454: if (detachedInitializationFailed) return; On 2016/01/06 08:41:57, sergei wrote: > it would be better to use some method `bool IsInitialized() const`, it's a bit > confusing that we are checking only result of "detached" initialization. I don't disagree in principle, but that change is premature. I think that particular function may not even be what we want, not exactly. The flag "detachedInitializationFailed" is really a proxy for something more like "IsPluginDisabledOnTechnicalGrounds()". We're not ready to write such a function yet. There are issue around transient vs. permanent failures, for example, that we're nowhere close to resolving. We have something like four different kinds of initialization for 'CPluginClass'. They are not all working well yet. We're going to have to live with some rather more explicit checking on this issue until we've got all the pieces in place. It's not going to happen instantly. Anticipating this class of concerns, I created #3433 a few weeks ago on this topic. Addressing that issue is going to be multi-commit process, I'm sure. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:54: class AtlEventRegistration On 2016/01/06 08:41:58, sergei wrote: > I doubt that we need two classes here, AtlEventRegistrationHolder and > AtlEventRegistration, only one would be completely enough. The code is correct > but it's overengineering. It's not overengineering. You actually want two classes here. The underlying class is the RAII class. The other one with Start() and Stop() also needs to implicitly call Stop() in its destructor. If you combined them, you'd end up with the equivalent of the RAII destructor code as a private method called from two places. It could be made to work, but it's error-prone. The present code is obviously exception-safe by design. That's why you split out a resource class into its own class and, if it needs some control sequence other than construction/destruction, why you write such a control class separately. This is an example of the Pimpl idiom, here not done so much to hide the implementation but more to get reliable destructor semantics. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:74: throw std::runtime_error("DispEventAdvise() failed"); On 2016/01/06 08:41:57, sergei wrote: > I would prefer to make private constructor and have a static creator method > (e.g. > `static std::unique_ptr<AtlEventRegistration<Sink, Source>> Create(Sink* > consumer, Source* producer);`) instead of AtlEventRegistrationHolder and such > usage of exceptions. > > In addition we can have a public typedef for > `std::unique_ptr<AtlEventRegistration<Sink, Source>>`. I consider this suggestion an inferior design. See my previous comment for why. Also see "Fear of adding classes". https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:115: : impl(nullptr) On 2016/01/06 08:41:58, sergei wrote: > This line as well as the empty constructor and destructor are not needed. Done. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:129: impl = nullptr; On 2016/01/06 08:41:58, sergei wrote: > This line is not needed. It is. Start() has the semantics of stopping whatever was previously there. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:137: impl = nullptr; On 2016/01/06 08:41:58, sergei wrote: > it's correct, but I find it better to use impl.reset(); Done. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:241: * Event manager for events coming from our site object. On 2016/01/06 08:41:58, sergei wrote: > Why do we need this comment? Because we have multiple sources of events. We have UI events, which should be and might eventually be split out. But we're also anticipating having this class handle 'HTMLDocumentEvents2Listener' events, which would use a second instance of this new class. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:244: bool detachedInitializationFailed; On 2016/01/06 08:41:58, sergei wrote: > The member naming is not consistent with other members because "m_" prefix is > missed. Right. It's not there because "m_" prefixes are against our style guidelines, and this is new code. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:244: bool detachedInitializationFailed; On 2016/01/06 08:41:58, sergei wrote: > Since it's accessed from different threads, there should be a proper access > methods, e.g. it can be std::atomic<bool> or protected by some synchronization > primitive if we actually need this member. You're raising a valid issue, certainly, but I've already solved it elsewhere. See https://codereview.adblockplus.org/29332660/, which contains exactly such primitives, and also the review dependent upon it that uses those primitives. I wrote that code first, and this member variable is essentially a placeholder for "EnsureInitialized()" from a new detached initialization facility. I'd prefer to get those two reviews done first, frankly, but I'm not holding out any particular expectation for prompt and speedy reviewing. (Much less converting the rest of the detached initialization.) The present change set could be committed separately with the caveat that we need to kill off the legacy detached initialization thread before any public release.
I'm waiting for the opinion of others
On 2016/01/08 10:14:29, sergei wrote: > I'm waiting for the opinion of others I'm waiting for your opinion. If you have an objection I have not explicitly addressed, then say so. You have failed to make any argument about why the present code is wrong. If you are unable to make such an argument, then you should not stand in the way of making progress. Register your unease and we can move on.
Let's compare the case with AtlEventRegistrationHolder and without.
`connection` refers to `std::unique_ptr<AtlEventRegistration<Sink, Source>>`
with
without
AtlEventRegistrationHolder::ctr\ underlying connection is nullptr
variable responsibe for connection is nullptr
connection::ctr
AtlEventRegistrationHolder::dtr\ means disconnect means
disconnect
connection::dtr/reset(nullptr) dtr of connection is called dtr of
connection is called
AtlEventRegistrationHolder::Start\ instantiates connection
instantiates connection
static AtlEventRegistration::Start
AtlEventRegistrationHolder::Stop means disconnect there
is no additional Stop method because it would
dtr of connection is called
duplicate disconnecting using destructor. To disconnect
one
can just destroy connection object.
So, the everything we need is to create and destroy connection object (that is
the core of RAII idea), AtlEventRegistrationHolder adds additional method Stop
to do it and adds method Start on the object with some state, these factors only
create questions for the reader what complicates the understanding and
maintaining of the code.
In addition current impl uses exceptions for the logic.
I think it would be clearer to suggest the preliminary implementation of Start
method:
private:
AtlEventRegistration(Sink* consumer, Source* producer)
: consumer(consumer), producer(producer)
{
}
public:
static std::unique_ptr<AtlEventRegistration<Sink, Source>> Connect(Sink*
listener, Source* source)
{
std::unique_ptr<AtlEventRegistration<Sink, Source>> retValue =
std::make_unique<AtlEventRegistration<Sink, Source>>(listener, source);
auto hr = retValue->listener->DispEventAdvise(retValue->source);
if (FAILED(hr))
{
DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE,
"AtlEventRegistration::Connect - Advise");
return std::unique_ptr<AtlEventRegistration<Sink, Source>>();
}
return retValue;
}
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:454: if (detachedInitializationFailed) return;
Acknowledged.
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
File src/plugin/PluginClass.h (right):
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
src/plugin/PluginClass.h:54: class AtlEventRegistration
On 2016/01/07 16:24:48, Eric wrote:
> On 2016/01/06 08:41:58, sergei wrote:
> > I doubt that we need two classes here, AtlEventRegistrationHolder and
> > AtlEventRegistration, only one would be completely enough. The code is
correct
> > but it's overengineering.
>
> It's not overengineering. You actually want two classes here. The underlying
> class is the RAII class. The other one with Start() and Stop() also needs to
> implicitly call Stop() in its destructor. If you combined them, you'd end up
> with the equivalent of the RAII destructor code as a private method called
from
> two places. It could be made to work, but it's error-prone.
>
> The present code is obviously exception-safe by design. That's why you split
out
> a resource class into its own class and, if it needs some control sequence
other
> than construction/destruction, why you write such a control class separately.
>
Answered in the message.
> This is an example of the Pimpl idiom, here not done so much to hide the
> implementation but more to get reliable destructor semantics.
It's not a Pimpl idiom here and we don't need it here.
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
src/plugin/PluginClass.h:74: throw std::runtime_error("DispEventAdvise()
failed");
On 2016/01/07 16:24:48, Eric wrote:
> On 2016/01/06 08:41:57, sergei wrote:
> > I would prefer to make private constructor and have a static creator method
> > (e.g.
> > `static std::unique_ptr<AtlEventRegistration<Sink, Source>> Create(Sink*
> > consumer, Source* producer);`) instead of AtlEventRegistrationHolder and
such
> > usage of exceptions.
> >
> > In addition we can have a public typedef for
> > `std::unique_ptr<AtlEventRegistration<Sink, Source>>`.
>
> I consider this suggestion an inferior design. See my previous comment for
why.
>
> Also see "Fear of adding classes".
I consider the current impl as overengineering and I don't like that exceptions
are used for the logic.
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
src/plugin/PluginClass.h:129: impl = nullptr;
On 2016/01/07 16:24:48, Eric wrote:
> On 2016/01/06 08:41:58, sergei wrote:
> > This line is not needed.
>
> It is. Start() has the semantics of stopping whatever was previously there.
Actually expecting that Start can be called several times looks redundant, just
one class with factory method as proposed would be better.
https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl...
src/plugin/PluginClass.h:241: * Event manager for events coming from our site
object.
On 2016/01/07 16:24:48, Eric wrote:
> On 2016/01/06 08:41:58, sergei wrote:
> > Why do we need this comment?
>
> Because we have multiple sources of events.
>
> We have UI events, which should be and might eventually be split out. But
we're
> also anticipating having this class handle 'HTMLDocumentEvents2Listener'
events,
> which would use a second instance of this new class.
I mean, it's clear from line `AtlEventRegistrationHolder<CPluginClass,
IWebBrowser2> browserEvents;`, why do we need to comment it? The comment
explains neither why we do it nor any unevident trick.
https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl...
File src/plugin/PluginClass.h (right):
https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl...
src/plugin/PluginClass.h:67: AtlEventRegistration(Sink* consumer, Source*
producer)
Just noticed, it would be better to call them as listener and source
respectively, because the domain is events.
One can find formatted message here http://pastebin.com/raw/brdxwVpw
On 2016/02/04 13:45:51, sergei wrote: > Let's compare the case with AtlEventRegistrationHolder and without. You know, I understood you the first time. You want to combine the two classes into one. Therefore, let me be completely clear: 1) Technically, that would work. 2) It is an inferior way to implement it. I believe I said both of these before. > So, the everything we need is to create and destroy connection object (that is > the core of RAII idea), RAII in C++ also means throwing exceptions if initialization fails. You are proposing something that is not RAII in C++. RAII has two complementary sides: (-1) If the resource was not successfully allocated, then no object exists because the constructor threw an exception. (+1) If the resource was successfully allocated, then an object exists for it because the constructor returned ordinarily. The reason for these invariants is that you can use multiple resources in an class by simple declaration as member variables. The exception unwinding algorithm, an old part of the C++ standard, guarantees atomicity of the composite. If you don't avail yourself of this facility, you're writing C with C++ syntax. * 'AtlEventRegistration' is a resource class that uses the standard RAII pattern. This class should not need alteration in future maintenance, because it has the correct boundary around a single resource. * 'AtlEventRegistrationHolder' manages the resource. Perhaps 'AtlEventRegistrationManager' would be a better name. Regardless, this class is intended to evolve as the code evolves. I have mentioned in passing before that 'SetSite()' could be better implemented as managing the lifetime of an instance of a "class X" that does all the "real work" of the plugin. (More or less, that's what 'CPluginTab' should be, but currently that work is arbitrarily split up between 'CPluginClass' and 'CPluginTab'.) Class X would have, as member variables, each of the resources that are are currently being set up explicitly in 'SetSite()'. One of those resources would be a member of type 'AtlEventRegistration'. The life cycle of that member would be managed the constructor/destructor of class X. There would be no need for 'AtlEventRegistrationHolder' at all and it would be removed. As the code exists today, implementing a class X is not a realistic, immediate goal. Nevertheless, it remains a good ultimate goal, because structuring the code that way eliminates the possibility of a whole class of null pointer errors related to mismatches of lifetime. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:54: class AtlEventRegistration On 2016/02/04 13:45:50, sergei wrote: > It's not a Pimpl idiom here and we don't need it here. Perhaps you are unclear about PImpl. PImpl = Pointer to Implementation PImpl class = AtlEventRegistrationHolder pointer = AtlEventRegistrationHolder::impl (a smart pointer) implementation = AtlEventRegistration Most PImpl classes use facade methods to forward methods to the underlying implementation. This PImpl class differs in that it has no methods to forward; either the event connection exists or does not. It's also ordinary for the lifetime of the PImpl class to coincide with that of the implementation. This PImpl class differs because it explicitly manages that lifetime. The most conventional reason to use PImpl is to get efficient pass-by-value. Just because this class doesn't act that way doesn't mean it's not PImpl. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:74: throw std::runtime_error("DispEventAdvise() failed"); On 2016/02/04 13:45:49, sergei wrote: > I consider the current impl as overengineering Yes, I know. You said that already. > I don't like that exceptions > are used for the logic. If you don't like exceptions, then you can't like RAII. Exceptions are at the core of how RAII works. If initialization fails, you throw an exception. If you don't, you're falling outside of the RAII pattern. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:241: * Event manager for events coming from our site object. On 2016/02/04 13:45:49, sergei wrote: > I mean, it's clear from line `AtlEventRegistrationHolder<CPluginClass, > IWebBrowser2> browserEvents;`, why do we need to comment it? It's a Doxygen comment. https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl... src/plugin/PluginClass.h:67: AtlEventRegistration(Sink* consumer, Source* producer) On 2016/02/04 13:45:50, sergei wrote: > Just noticed, it would be better to call them as listener and source > respectively, because the domain is events. Neither pair of words "source/sink" and "listener/source" is particularly great English. "Consume/produce" is better. If you'd like to get into the fine points of English connotation, I can expound.
I see the point. You want to have some class which is instantiated when SetSite(not nullptr) is called and destroyed when SetSite(nullptr) is called. If we cannot initialize some parts, e.g. subscribe to events, then that class won't be instantiated. And to have less changes in future you want to have ready to use AtlEventRegistration, thus it should throw an exception in constructor, and just drop AtlEventRegistrationHolder later. That's fine and I still don't see the reason we need AtlEventRegistrationHolder now. Currently all exceptions thrown by AtlEventRegistration::ctr are caught and don't propagate upward, what will change if replace AtlEventRegistrationHolder by std::unique_ptr<AtlEventRegistration>? - Nothing. If you still want to hide all exceptions from the constructing of AtlEventRegistration then we can just add static factory method to AtlEventRegistration which catches the exceptions, it will already simplify it (as it's said above having additional Start and Stop methods on the object with some state only complicate), but why do we want to "eat" those exceptions? Since we disagree here, I would let Oleksandr to decide it. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:54: class AtlEventRegistration On 2016/02/04 17:00:27, Eric wrote: > On 2016/02/04 13:45:50, sergei wrote: > > It's not a Pimpl idiom here and we don't need it here. > > Perhaps you are unclear about PImpl. > > PImpl = Pointer to Implementation > PImpl class = AtlEventRegistrationHolder > pointer = AtlEventRegistrationHolder::impl (a smart pointer) > implementation = AtlEventRegistration > > Most PImpl classes use facade methods to forward methods to the underlying > implementation. This PImpl class differs in that it has no methods to forward; > either the event connection exists or does not. It's also ordinary for the > lifetime of the PImpl class to coincide with that of the implementation. This > PImpl class differs because it explicitly manages that lifetime. > > The most conventional reason to use PImpl is to get efficient pass-by-value. > Just because this class doesn't act that way doesn't mean it's not PImpl. I don't want to debate here, but it's not PIMPL, I would consider it as merely a composition. Patterns and idioms are not merely some, let's say, UML diagram. They also include in particular the intention to use it and the way to organize files and code. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:74: throw std::runtime_error("DispEventAdvise() failed"); On 2016/02/04 17:00:27, Eric wrote: > On 2016/02/04 13:45:49, sergei wrote: > > I consider the current impl as overengineering > > Yes, I know. You said that already. > > > I don't like that exceptions > > are used for the logic. > > If you don't like exceptions, then you can't like RAII. Exceptions are at the > core of how RAII works. If initialization fails, you throw an exception. If you > don't, you're falling outside of the RAII pattern. > I have not said whether I like exceptions or not, I said that I don't like when they are used to build some logic. It does not matter whether somebody likes exceptions or not, I'm trying to communicate that it's overengineering and we are trying to avoid it. https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl... src/plugin/PluginClass.h:67: AtlEventRegistration(Sink* consumer, Source* producer) On 2016/02/04 17:00:28, Eric wrote: > On 2016/02/04 13:45:50, sergei wrote: > > Just noticed, it would be better to call them as listener and source > > respectively, because the domain is events. > > Neither pair of words "source/sink" and "listener/source" is particularly great > English. "Consume/produce" is better. If you'd like to get into the fine points > of English connotation, I can expound. I think we should not write the code how it would be written by Shakespeare. There is a common terms in certain domains and we should stay with that terms to have the code maintainable. Terms "consumer" and "producer" better fit producer–consumer problem, which is about transferring data from producer to consumer using different types of shared buffers. Terms "listener" and "source" or "sender" better fit to the events domain. Compare the frequency of usage https://github.com/search?l=cpp&q=event+producer+consumer&ref=searchresults&t... https://github.com/search?l=cpp&q=event+listener+source&ref=searchresults&typ... https://github.com/search?l=cpp&q=event+listener+sender&ref=searchresults&typ...
On 2016/02/04 19:41:58, sergei wrote: > I see the point. You want to have some class which is instantiated when > SetSite(not nullptr) is called and destroyed when SetSite(nullptr) is called. That is _one_ of the desiderata. If that's all that you see, you are indeed missing the whole point. > thus it should throw an exception in constructor, and > just drop AtlEventRegistrationHolder later. > That's fine OK. Seems like progress. You've said you're OK with 'AtlEventRegistration'. So the real question is then whether we need a separate class or whether we would inline its code. I admit that inlining its code would work, because each of those functions is only called once. I believe having a second class is better because it clarifies the code to 'SetSite()', which is still something of an unholy mess and greatly benefits from clarity. Removing the details of lifecycle management out of 'SetSite()' is a benefit worth paying for by having a new class. I'll note for Oleksandr's benefit that you don't seem to have identified any defects with the code and that your only remaining concerns are with code style, broadly construed. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:54: class AtlEventRegistration On 2016/02/04 19:41:57, sergei wrote: > I don't want to debate here I don't want to debate here either, so I'll forgo a response. https://codereview.adblockplus.org/29332848/diff/29333238/src/plugin/PluginCl... src/plugin/PluginClass.h:74: throw std::runtime_error("DispEventAdvise() failed"); On 2016/02/04 19:41:57, sergei wrote: > I have not said whether I like exceptions or not, I said that I don't like when > they are used to build some logic. You'll have to tell me what "logic" you're talking about then. The constructor of this class throws when it fails to acquire a resource. If you have a problem with that, you have a problem with RAII generally. https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29332848/diff/29333294/src/plugin/PluginCl... src/plugin/PluginClass.h:67: AtlEventRegistration(Sink* consumer, Source* producer) We've entered bikeshedding territory here. You have a personal preference that differs from mine. Suggestion rejected. 
|
