Issue 29323561: Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass (Closed)
DescriptionIssue #3383 - Rewrite and simplify browser-site handling in CPluginClass.
Change type of 'm_webBrowser2' from 'CComQIPtr' to 'CComPtr' and also make it
private. Remove 'GetBrowser()', which was not called outside this class. Its
synchronization behavior is also no longer needed. Remove its synchronization
member 's_criticalSectionBrowser'.
Rewrite all occurrences of 'GetBrowser()' with direct references to
'm_webBrowser2'. Add 'IsRootBrowser()', which encapsulates otherwise-
duplicated code (and reads better).
Dead code. Remove 'FinalRelease()', which is no longer needed. Remove
'FinalConstruct()', which was providing nothing over its base class
implementation. Remove 'GetTabHWND()', which was never called. Remove
'MainThreadProc()', 'GetMainThreadHandle()', and 'IsMainThreadDone()',
none of which were even defined.
Patch Set 1 #
Total comments: 14
Patch Set 2 : fixed assignment of m_webBrowser2 in SetSite #Patch Set 3 : change type of m_webBrowser2 #
Total comments: 22
Patch Set 4 : address comments #
Total comments: 4
Patch Set 5 : initialization; shorten comment #MessagesTotal messages: 18
https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:320: IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite); Moving this statement outside the try-block ensures it's always called, which matters more now since the new code here contains an explicit exception throw.
It seems better to split it at least into removing of dead code and the rest. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:335: m_webBrowser2.Release(); Why is this line removed? https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:140: CComPtr<IWebBrowser> thisBrowser = m_webBrowser2; NIT: why is `IWebBrowser` and not `IWebBrowser2`? https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:220: */ I'm sorry, but this comment seems just wrong. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:221: hr = unknownSite->QueryInterface(IID_IWebBrowser2, (void**)(&m_webBrowser2)); I cannot find the call of `Release` on `m_webBrowser2`. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.h:141: IWebBrowser2* m_webBrowser2; I really don't understand the reluctance to use smart pointers. The one point here is that it should be without QI.
https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:220: */ On 2015/10/01 16:15:51, sergei wrote: > I'm sorry, but this comment seems just wrong. Whatever the base class SetSite does it doesn't know that we have called QueryInterface and stored a pointer here. So we still need to AddRef here. Which is why I agree that we should still use CComPtr for m_webBrowser2. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.h:141: IWebBrowser2* m_webBrowser2; On 2015/10/01 16:15:51, sergei wrote: > I really don't understand the reluctance to use smart pointers. The one point > here is that it should be without QI. +1.
https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:220: */ On 2015/10/05 10:44:47, Oleksandr wrote: > On 2015/10/01 16:15:51, sergei wrote: > > I'm sorry, but this comment seems just wrong. > > Whatever the base class SetSite does it doesn't know that we have called > QueryInterface and stored a pointer here. So we still need to AddRef here. Which > is why I agree that we should still use CComPtr for m_webBrowser2. The base class calls `AddRef` and corresponding `Release` and does not know that we have called `QueryInterface` here, right, but `QueryInterface` also calls `AddRef`, so we need to have corresponding call `Release`. We still can call `Release` manually (as it was before) but smart pointer ensures that it will be called when the class is being destroyed, so it is the first reason we need to use smart pointer. The second one is that in particular the implementation of `ATL::CComPtr` has a couple of good caveats useful during in debug build (dereference operators and `CComPtr::operator&`) and in runtime (`CComPtr::constructor` and `CComPtr::Release`).
New patch set also renames IsRootPageBrowser() to IsRootBrowser(). https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:140: CComPtr<IWebBrowser> thisBrowser = m_webBrowser2; On 2015/10/01 16:15:51, sergei wrote: > NIT: why is `IWebBrowser` and not `IWebBrowser2`? Typo. Fixed. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:220: */ On 2015/10/05 11:00:56, sergei wrote: > [...] but smart pointer ensures that it will be > called when the class is being destroyed, so it is the first reason we need to > use smart pointer. I've done this in the new patch set. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:220: */ On 2015/10/01 16:15:51, sergei wrote: > I'm sorry, but this comment seems just wrong. The base class SetSite() assigns its argument 'unknownSite' to an internal member of type 'CComPtr<IUnknown>'. So if there's any lingering concern that we're not properly managing lifetime, the ATL base class implementation is taking care of that silently. I've rewritten the comment anyway, because that issue isn't particularly relevant. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.cpp:221: hr = unknownSite->QueryInterface(IID_IWebBrowser2, (void**)(&m_webBrowser2)); On 2015/10/01 16:15:51, sergei wrote: > I cannot find the call of `Release` on `m_webBrowser2`. Done. https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... src/plugin/PluginClass.h:141: IWebBrowser2* m_webBrowser2; On 2015/10/01 16:15:51, sergei wrote: > I really don't understand the reluctance to use smart pointers. My rule: Use them when you need them and don't use them when you don't need them. In addition to a performance improvement, the code expresses intent better. Using CComPtr object as a class member clearly expresses an intent that it's holding a reference to a COM object (by means of its reference increment) so that the COM object is not destroyed. This is relevant _only_ when the lifespan of the COM object does not encompass the lifetime of the member. Assertion: We don't need CComPtr here because the IWebBrowser2 object encompasses that of CPluginClass. The IWebBrowser2 COM object in IE is the one that instantiates the BHO object (CPluginClass) through the factory method. Since the IWebBrowser2 object correctly uses SetSite(), we'll receive both a call to SetSite(non-nullptr) before it calls anything else and a call to SetSite(0) after anything else and before it's destroyed. Hence the life span of the IWebBrowser2 completely encompasses that of CPluginClass.
On 2015/11/18 13:57:31, Eric wrote: > New patch set also renames IsRootPageBrowser() to IsRootBrowser(). > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > File src/plugin/PluginClass.cpp (right): > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > src/plugin/PluginClass.cpp:140: CComPtr<IWebBrowser> thisBrowser = > m_webBrowser2; > On 2015/10/01 16:15:51, sergei wrote: > > NIT: why is `IWebBrowser` and not `IWebBrowser2`? > > Typo. Fixed. > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > src/plugin/PluginClass.cpp:220: */ > On 2015/10/05 11:00:56, sergei wrote: > > [...] but smart pointer ensures that it will be > > called when the class is being destroyed, so it is the first reason we need to > > use smart pointer. > > I've done this in the new patch set. > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > src/plugin/PluginClass.cpp:220: */ > On 2015/10/01 16:15:51, sergei wrote: > > I'm sorry, but this comment seems just wrong. > > The base class SetSite() assigns its argument 'unknownSite' to an internal > member of type 'CComPtr<IUnknown>'. So if there's any lingering concern that > we're not properly managing lifetime, the ATL base class implementation is > taking care of that silently. > > I've rewritten the comment anyway, because that issue isn't particularly > relevant. > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > src/plugin/PluginClass.cpp:221: hr = > unknownSite->QueryInterface(IID_IWebBrowser2, (void**)(&m_webBrowser2)); > On 2015/10/01 16:15:51, sergei wrote: > > I cannot find the call of `Release` on `m_webBrowser2`. > > Done. > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > File src/plugin/PluginClass.h (right): > > https://codereview.adblockplus.org/29323561/diff/29323562/src/plugin/PluginCl... > src/plugin/PluginClass.h:141: IWebBrowser2* m_webBrowser2; > On 2015/10/01 16:15:51, sergei wrote: > > I really don't understand the reluctance to use smart pointers. > > My rule: Use them when you need them and don't use them when you don't need > them. In addition to a performance improvement, the code expresses intent > better. > > Using CComPtr object as a class member clearly expresses an intent that it's > holding a reference to a COM object (by means of its reference increment) so > that the COM object is not destroyed. This is relevant _only_ when the lifespan > of the COM object does not encompass the lifetime of the member. > > Assertion: We don't need CComPtr here because the IWebBrowser2 object > encompasses that of CPluginClass. > > The IWebBrowser2 COM object in IE is the one that instantiates the BHO object > (CPluginClass) through the factory method. Since the IWebBrowser2 object > correctly uses SetSite(), we'll receive both a call to SetSite(non-nullptr) > before it calls anything else and a call to SetSite(0) after anything else and > before it's destroyed. Hence the life span of the IWebBrowser2 completely > encompasses that of CPluginClass. It's really tedious to explain that we only benefit from using ATL::CComPtr for m_webBrowser member. You have just given a link in another codereview https://msdn.microsoft.com/en-us/library/ms810016.aspx which says > From a COM client's perspective, reference-counting is always a per-interface concept. Clients should never assume that an object uses the same reference count for all interfaces. Anyway, if you really care so much about each AddRef/Release could you calculate how many times it's called by us when the member is ATL::CComPtr and in the proposed code on some web site?
https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:335: m_webBrowser2.Release(); It's still good to call `m_webBrowser2.Release();` here at this point. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) We don't need it because ATL::CComPtr default constructor initializes internal pointer to nullptr. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; it would be good to initialize it to NULL. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:129: DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed") Actually, I think we should return nullptr in case of error, shouldn't we? https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.h:144: * It's values are set and reset solely in SetSite(). This part of the comment is not necessary, it's clear from the aim of ATL::CComPtr.
New patch set incorporates a rebase also. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:335: m_webBrowser2.Release(); On 2015/11/30 15:52:08, sergei wrote: > It's still good to call `m_webBrowser2.Release();` here at this point. Basically right. The correct thing to do is to nullify the smart pointer. That will trigger a call to 'Release()' on the interface in the destructor of 'm_webBrowser2'. No need to break abstraction here. For the record, I've verified that IE always calls the destructor for 'CPluginClass' immediately after calling 'SetSite(0)' on it (except sometimes on program termination). If we did not nullify the pointer here, it would be destroyed anyway shortly after in the destructor for 'CPluginClass'. Nevertheless, it's good to make everything about this variable as explicit as possible. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) On 2015/11/30 15:52:08, sergei wrote: > We don't need it because ATL::CComPtr default constructor initializes internal > pointer to nullptr. I'd rather have it explicit as a lightweight form of documentation. The lifetime of that object is inherently problematic if you're not deeply familiar with how IE uses SetSite. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; On 2015/11/30 15:52:09, sergei wrote: > it would be good to initialize it to NULL. Welcome to MicrosoftLand. SHANDLE_PTR is not a pointer type; it's 'long'. We're not testing its value. Since we're not returning 'nullptr' on error (see below), we're only returning its value if 'get_HWND()' succeeds, and in that case we can rely upon its value. I'd say we don't need to bother to initialize it, particularly since initializing it with 'nullptr', what looks on the surface like it would be the obvious choice, doesn't compile. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:129: DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed") On 2015/11/30 15:52:08, sergei wrote: > Actually, I think we should return nullptr in case of error, shouldn't we? Done. That's a good idea. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.h:144: * It's values are set and reset solely in SetSite(). On 2015/11/30 15:52:09, sergei wrote: > This part of the comment is not necessary, it's clear from the aim of > ATL::CComPtr. I disagree. Nothing about this variable is either clear or obvious.
Overall a welcome change. One note I'd like to mention though is that FinalConstruct was in fact used and was called by the IE. It was a dummy implementation though, so it was rightfully removed. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; On 2015/11/30 16:31:51, Eric wrote: > On 2015/11/30 15:52:09, sergei wrote: > > it would be good to initialize it to NULL. > > Welcome to MicrosoftLand. SHANDLE_PTR is not a pointer type; it's 'long'. > > We're not testing its value. Since we're not returning 'nullptr' on error (see > below), we're only returning its value if 'get_HWND()' succeeds, and in that > case we can rely upon its value. > > I'd say we don't need to bother to initialize it, particularly since > initializing it with 'nullptr', what looks on the surface like it would be the > obvious choice, doesn't compile. We don't check its value now, but we might in future. So I don't see why we would remove the initialization here. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.h:144: * It's values are set and reset solely in SetSite(). On 2015/11/30 16:31:51, Eric wrote: > On 2015/11/30 15:52:09, sergei wrote: > > This part of the comment is not necessary, it's clear from the aim of > > ATL::CComPtr. > > I disagree. Nothing about this variable is either clear or obvious. I assume we're talking about the part of the comment below, not above. I agree it looks redundant - CComPtr usage is always a defensive programming. But this being just a comment I don't mind leaving it here.
New patch set. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; Initialized it to zero, which matches its type. As a rule, though, if we're going to do something in the future, we can do its prerequisites in the future also. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.h (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.h:144: * It's values are set and reset solely in SetSite(). On 2015/12/03 11:51:58, Oleksandr wrote: > CComPtr usage is always a defensive programming. It's no more defensive, at least ordinarily, than using 'unique_ptr' or 'shared_ptr' is. In the present case it's compensating for a poor interface design in SetSite(), which takes an 'IUnknown*' and contains no further type information, even though it's always present in the caller. I've edited down the comment. https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: } On 2015/12/03 11:51:59, Oleksandr wrote: > One note I'd like to mention though is that > FinalConstruct was in fact used and was called by the IE. It was a dummy > implementation though, so it was rightfully removed. I've edited the commit message accordingly. To be precise, it's our own code that calls it. It's called as a result of a COM class factory call 'CreateInstance'; the implementation is within ATL. Any other COM client that constructed 'CPluginClass' would also call 'FinalConstruct'. And yes, the dummy implementation was exactly the same as the code in our base class 'CComObjectRootEx', which is to do nothing. The real reason, though, we don't need to be using 'FinalConstruct' & 'FinalRelease' is that we've got we are subsidiary to browser objects and we have a reliable call sequence to 'SetSite' that bounds the lifetime of that utility to the browser.
The patch looks good, however I think the change is too substantial for a Noissue commit. It will be difficult to find this codereview based on the changeset alone. I would either create an issue and pasted codereview URL there, or maybe add a link to the codereview in a commit message. The later seems like a non-standard way of doing things, so having an issue for this would be best, IMO.
On 2015/12/04 11:19:09, Oleksandr wrote: > The patch looks good, however I think the change is too substantial for a > Noissue commit. I agree with this. Indeed, this change is just one part of a larger effort to clean up all the life cycle and initialization issues. There are the three I mentioned responding to the CSS injection review (this one, the 's_plugininstances' one, the namespaces registration one). There's another one I haven't posted code yet involving initialization of 'CPluginFilter' (its filter loading thread). We have an unreported defect with 'DllCanUnloadNow()' that I've seen occasionally generate APPCRASH. There's a comment in the code about waiting needing to join a thread in the destructor. What I'm saying is that we not only need an issue for this one, we should probably also make a meta-issue for all of them.
https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) On 2015/11/30 16:31:51, Eric wrote: > On 2015/11/30 15:52:08, sergei wrote: > > We don't need it because ATL::CComPtr default constructor initializes internal > > pointer to nullptr. > > I'd rather have it explicit as a lightweight form of documentation. The lifetime > of that object is inherently problematic if you're not deeply familiar with how > IE uses SetSite. ATL::CComPtr initializes it to nullptr in the constructor, what does this initialization document here in addition and how is it related to the lifetime of some object? I'm also not sure which object is referred to above. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; On 2015/12/03 14:24:54, Eric wrote: > ... > Welcome to MicrosoftLand. SHANDLE_PTR is not a pointer type; it's 'long'. > ... > Initialized it to zero, which matches its type. Although zero is OK, NULL looks better here. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; On 2015/12/03 11:51:58, Oleksandr wrote: > On 2015/11/30 16:31:51, Eric wrote: > > On 2015/11/30 15:52:09, sergei wrote: > > > it would be good to initialize it to NULL. > > > > Welcome to MicrosoftLand. SHANDLE_PTR is not a pointer type; it's 'long'. > > > > We're not testing its value. Since we're not returning 'nullptr' on error (see > > below), we're only returning its value if 'get_HWND()' succeeds, and in that > > case we can rely upon its value. > > > > I'd say we don't need to bother to initialize it, particularly since > > initializing it with 'nullptr', what looks on the surface like it would be the > > obvious choice, doesn't compile. > > We don't check its value now, but we might in future. So I don't see why we > would remove the initialization here. On 2015/12/03 14:24:54, Eric wrote: > ... > As a rule, though, if we're going to do something in the future, we can do its > prerequisites in the future also. Agree, except following some good practices, like here. Beside the fact that it's always good to initialize variables, I would like to say it's a good practice to always initialize the value which is returned through a pointer, like here or like e.g. void** in QueryInterface, because it allows to find a bug during the development cycle. Such pointers are often held by some smart-pointer wrappers, so the callee can in addition verify that the value stored at the pointer value is not a valid value to avoid resource leakage by overriding it. Although one can argue that it's not our case because we are calling Microsoft's method (which in particular does not fail or trigger any assert depending on the argument value) and we are not going to implement tests for our plugin, I still think that it's worth doing because at least it simplifies the readability because one don't have to think and look into whether it's our implementation or can somehow fail later and of course it does not cause any questions during the codereview. https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: } On 2015/12/03 14:24:54, Eric wrote: > On 2015/12/03 11:51:59, Oleksandr wrote: > > One note I'd like to mention though is that > > FinalConstruct was in fact used and was called by the IE. It was a dummy > > implementation though, so it was rightfully removed. > > I've edited the commit message accordingly. > > To be precise, it's our own code that calls it. It's called as a result of a COM > class factory call 'CreateInstance'; the implementation is within ATL. Any other > COM client that constructed 'CPluginClass' would also call 'FinalConstruct'. > > And yes, the dummy implementation was exactly the same as the code in our base > class 'CComObjectRootEx', which is to do nothing. > > The real reason, though, we don't need to be using 'FinalConstruct' & > 'FinalRelease' is that we've got we are subsidiary to browser objects and we > have a reliable call sequence to 'SetSite' that bounds the lifetime of that > utility to the browser. I disagree that we don't need them, they are really useful and a common ATL convention. Instead of removing FinalConstruct, it would be better to move constructing of `CPluginTab(this)` and the call `Dictionary::Create(GetBrowserLanguage());` into this method, wrap by exceptions catcher and return e.g. E_FAIL in case of exception. It's a common way to friendly fail constructing of an instance of COM class object with ATL. In addition pay attention that during FinalConstruct and FinalRelease the object is fully constructed and all virtual methods are available, for more info see the ATL::CComObject which inherits CPluginClass in our case. As well as in addition I highly recommend to get acquainted with the reason we should use DECLARE_PROTECT_FINAL_CONSTRUCT, I don't think it makes sense to explain it in the codereview. BTW, using of that macro makes no sense without custom implementation of FinalConstruct in our case. One may argue that we are not using our COM-essence in the members of CPluginClass now, but I'm pretty sure if we start it in a future or do it somehow accidentally we will forget about peculiarities covered by FinalDestruct and FinalConstruct. Of course it can be also considered as a defensive programming but for me not using of them looks rather as a complication.
I've addressed Oleksandr's comment; there's now an issue for this change set. The most recent comments from Sergei are two very minor points about code style, both of which I've addressed, and one much larger suggestion which is out of scope. It looks like we're done here. Are we done yet? https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) On 2015/12/07 10:49:08, sergei wrote: > ATL::CComPtr initializes it to nullptr in the constructor, what does this > initialization document here in addition The very-lightweight documentation is that every value that 'm_webBrowser2' takes appears explicitly in the code, including the initial value of zero. > and how is it related to the lifetime > of some object? I'm also not sure which object is referred to above. It's related to the life cycle (not really the life span) of 'CPluginClass'. FYI, this is how I think about this issue. Knowing what I know now, if I were implementing this from scratch, I would move much of what's currently in 'CPluginClass' to a new class that would be created/destroyed in 'SetSite()'. (I'm not planning on introducing such a change.) The way I see it, there is, effectively, a second class with a slightly narrower lifetime whose constructor has one argument of type 'IWebBrowser2'. But there's a disconnect between (1) a C++ constructor with one argument and (2) the 'SetSite' implementation pattern. These could be bridged explicitly, but doing so at this point would be quite an extensive change. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; On 2015/12/07 10:49:08, sergei wrote: > Beside the fact that it's always good to initialize variables, Almost always. But even saying that is saying too much here, because there's no remaining issue of initialization. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: SHANDLE_PTR hBrowserWndHandle; On 2015/12/07 10:49:08, sergei wrote: > Although zero is OK, NULL looks better here. I disagree. Sometimes NULL is defined as '(void*)0'. I don't want to incur any cognitive overhead thinking about what it ought to be. It's Microsoft's interface defect; we have to live with it. https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: } On 2015/12/07 10:49:08, sergei wrote: > I disagree that we don't need them, they are really useful and a common ATL > convention. If you want to use them here, please submit a subsequent change set. Two phase constructors/destructors are widely considered something of an anti-pattern in C++. With COM, sometimes you're stuck using them because COM doesn't use C++ patterns. There is, however, no need to use them in the present code. They are absent from the present code. Any further discussion is about future code, not the present review. I have no further desire to discuss this here. I want to get this change committed. It's blocking progress, including a number of changes to life cycle and event registration to get CSS injection working.
Less code less bugs, I think. LGTM.
LGMT https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (right): https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) On 2015/12/07 14:04:12, Eric wrote: > On 2015/12/07 10:49:08, sergei wrote: > > ATL::CComPtr initializes it to nullptr in the constructor, what does this > > initialization document here in addition > > The very-lightweight documentation is that every value that 'm_webBrowser2' > takes appears explicitly in the code, including the initial value of zero. It sounds not convincing. Why are we not doing it everywhere? > > and how is it related to the lifetime > > of some object? I'm also not sure which object is referred to above. > > It's related to the life cycle (not really the life span) of 'CPluginClass'. > > FYI, this is how I think about this issue. Knowing what I know now, if I were > implementing this from scratch, I would move much of what's currently in > 'CPluginClass' to a new class that would be created/destroyed in 'SetSite()'. > (I'm not planning on introducing such a change.) The way I see it, there is, > effectively, a second class with a slightly narrower lifetime whose constructor > has one argument of type 'IWebBrowser2'. But there's a disconnect between (1) a > C++ constructor with one argument and (2) the 'SetSite' implementation pattern. > These could be bridged explicitly, but doing so at this point would be quite an > extensive change. Sounds reasonable but it's not related to initialization of class members with the same values which they get from the default constructor. https://codereview.adblockplus.org/29323561/diff/29330400/src/plugin/PluginCl... src/plugin/PluginClass.cpp:93: : m_webBrowser2(nullptr) I think it will go away in a future and I don't understand the reason to introduce it to get rid of it later. https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... File src/plugin/PluginClass.cpp (left): https://codereview.adblockplus.org/29323561/diff/29331633/src/plugin/PluginCl... src/plugin/PluginClass.cpp:125: } On 2015/12/07 14:04:12, Eric wrote: > On 2015/12/07 10:49:08, sergei wrote: > > I disagree that we don't need them, they are really useful and a common ATL > > convention. > > If you want to use them here, please submit a subsequent change set. > > Two phase constructors/destructors are widely considered something of an > anti-pattern in C++. With COM, sometimes you're stuck using them because COM > doesn't use C++ patterns. There is, however, no need to use them in the present > code. They are absent from the present code. Any further discussion is about > future code, not the present review. > > I have no further desire to discuss this here. I want to get this change > committed. It's blocking progress, including a number of changes to life cycle > and event registration to get CSS injection working. Acknowledged.
On 2015/12/08 09:42:00, sergei wrote: > LGMT LGTM* 
|
