Opened on 02/27/2015 at 04:39:33 PM

Last modified on 03/13/2015 at 09:02:46 AM

#2058 new change

Remove thread associated with CPluginTab

Reported by: eric@adblockplus.org Assignee:
Priority: P4 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: Blocked By:
Blocking: Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Verified working:
Review URL(s):

Description

Background

The thread associated with CPluginTabBase no longer has anything specific to do with tabs. Instead, it's acting as a log processing pump, polling to perform deferred logging of errors. It's a violation of any number of good software practices.

  • It violates basic encapsulation in the worst way, using member variables in CPluginTabBase to implement a behavior used in error processing.
  • It's a needless use of threads, since we need at most one thread to deal with deferred logging, but we're using one thread per tab to do it.
  • It runs constantly as a polling loop, where it only needs to run when there are deferred errors to log.
  • In the case that the thread is told to stop running, it can leave deferred errors stranded in the queue. That is, there's nowhere where the queue is flushed at termination.

What to change

Remove the thread from CPluginTab. Eliminate member variables m_isActivated and m_continueThreadRunning, which are only used for the thread. Eliminate member function ThreadProc. Eliminate functions OnActivate and OnUpdate, which are only used to update m_isActivated.

There are a number of things that could be done at the same time, though none of them are, strictly speaking, necessary to fulfill the narrower goal of this ticket, which is simply to get the thread out of the class CPluginTab.

  • Encapsulate deferred error logging and its queue into a separate class.
  • Change from using a polling mechanism to one using notify/wait.

Attachments (0)

Change History (1)

comment:1 Changed on 03/13/2015 at 09:02:46 AM by oleksandr

  • Priority changed from Unknown to P4
  • Ready set

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.