Opened on 02/04/2016 at 02:27:26 PM

#3630 new change

Switch back to use FinalConstruct/FinalRelease

Reported by: sergz Assignee:
Priority: Unknown Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

Introduced in https://codereview.adblockplus.org/29323561/.

As it has been pointed in the comments they are really useful and a common ATL convention.

Instead of removing FinalConstruct, it would be better to move constructing of CPluginTab 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.

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.

What to change

  • restore these methods
  • move the code from constructor into FinalConstruct, wrap it by exceptions catcher and return e.g. E_FAIL in case of exception
  • move the code from destructor into FinalRelease and wrap it by exceptions catcher

If we decide to reject the issue

In case of rejection, to stay consistent we should also drop DECLARE_PROTECT_FINAL_CONSTRUCT from the header because using of that macro makes no sense without custom implementation of FinalConstruct in our case.

Attachments (0)

Change History (0)

Add Comment

Modify Ticket

Change Properties
Action
as new .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from (none).
Next status will be 'reviewing'.
 
Note: See TracTickets for help on using tickets.