Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29323611: Issue #1234, #2058 - Rewrite log facility, improving thread implementation

Created:
Aug. 13, 2015, 9:52 p.m. by Eric
Modified:
July 27, 2016, 10:24 p.m.
Reviewers:
sergei, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue #1234, #2058 - Rewrite log facility, improving thread implementation Replaced old class 'LogQueue' definition with an entirely new one based on the class 'ActiveQueue'. Added class 'LogQueueReference' to substitute for static initialization. Eliminate explicit use of process and thread ID outside the class 'LogEntry'. All log entries are consistently initialized with this information. Add location information to 'LogEntry'. This includes both the old pattern of numeric identifiers and a new one of source code identifiers. Added ActiveQueue.h and Placeholder.h, implementing a synchronized queue and an active queue, that is, one with an autonomous consumer. Added tests for these classes, which are segregated into their own project (for the time being) to enable development of the tests with a more recent toolset. Removed PluginClientBase.{h,cpp}, whose only remaining code was for the old log queue and its elements. Subsequently removed code only used therein. Removed CPluginSettings' member functions 'HasInstance()' and 'AddError()'. Removed 'CPluginTab' member variables 'm_thread', 'm_isActivated', and 'm_continueThreadRunning', member functions 'ThreadProc()', 'OnActivate()', 'OnUpdate()', and class function 'TabThreadProc()'. Removed unneeded SELFTEST macros.

Patch Set 1 : #

Total comments: 34

Patch Set 2 : rebase only #

Patch Set 3 : rebase to current tip #

Total comments: 12

Patch Set 4 : address comments #

Patch Set 5 : rebase only #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1026 lines, -340 lines) Patch
M adblockplus.gyp View 1 2 3 4 4 chunks +31 lines, -4 lines 0 comments Download
A src/plugin/ActiveQueue.h View 1 2 3 1 chunk +281 lines, -0 lines 0 comments Download
M src/plugin/Config.h View 3 chunks +6 lines, -10 lines 0 comments Download
A src/plugin/Placeholder.h View 1 chunk +78 lines, -0 lines 0 comments Download
M src/plugin/PluginClass.h View 1 2 3 4 2 chunks +6 lines, -0 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 2 3 4 7 chunks +6 lines, -7 lines 0 comments Download
R src/plugin/PluginClientBase.h View 1 2 3 1 chunk +0 lines, -72 lines 0 comments Download
R src/plugin/PluginClientBase.cpp View 1 2 3 1 chunk +0 lines, -84 lines 0 comments Download
M src/plugin/PluginDebug.h View 1 2 3 4 2 chunks +107 lines, -5 lines 0 comments Download
M src/plugin/PluginDebug.cpp View 1 2 11 chunks +181 lines, -35 lines 0 comments Download
M src/plugin/PluginMimeFilterClient.cpp View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/PluginMutex.cpp View 1 chunk +0 lines, -3 lines 0 comments Download
M src/plugin/PluginSettings.h View 1 2 chunks +0 lines, -3 lines 0 comments Download
M src/plugin/PluginSettings.cpp View 1 2 chunks +0 lines, -18 lines 0 comments Download
M src/plugin/PluginSystem.cpp View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M src/plugin/PluginTabBase.h View 1 2 2 chunks +0 lines, -8 lines 0 comments Download
M src/plugin/PluginTabBase.cpp View 1 2 3 4 chunks +0 lines, -89 lines 0 comments Download
A test/plugin/ActiveQueueTest.cpp View 1 chunk +183 lines, -0 lines 0 comments Download
A test/plugin/PlaceholderTest.cpp View 1 chunk +147 lines, -0 lines 0 comments Download

Messages

Total messages: 11
Eric
Aug. 19, 2015, 5:53 p.m. (2015-08-19 17:53:49 UTC) #1
sergei
Firstly, I have quickly gone through src/plugin/ActiveQueue.h file and have some comments there. Secondly, is ...
Oct. 1, 2015, 3:50 p.m. (2015-10-01 15:50:08 UTC) #2
Eric
On 2015/10/01 15:50:08, sergei wrote: > Firstly, I have quickly gone through src/plugin/ActiveQueue.h file and ...
Oct. 8, 2015, 9:05 p.m. (2015-10-08 21:05:41 UTC) #3
Eric
Note that this review depends upon a previous one: https://codereview.adblockplus.org/5187613258416128/ This review was created 7 ...
Oct. 9, 2015, 4:13 p.m. (2015-10-09 16:13:05 UTC) #4
Eric
New patch set is rebase only. It follows the rebase of its dependency https://codereview.adblockplus.org/5187613258416128/
Nov. 26, 2015, 1:15 p.m. (2015-11-26 13:15:34 UTC) #5
Eric
Patch set 3 is a rebase to current tip. It's dependency was just committed, so ...
Jan. 5, 2016, 2:50 p.m. (2016-01-05 14:50:51 UTC) #6
sergei
On 2016/01/05 14:50:51, Eric wrote: > Patch set 3 is a rebase to current tip. ...
Jan. 26, 2016, 11:28 p.m. (2016-01-26 23:28:43 UTC) #7
Oleksandr
> I disagree with the implementation of ActiveQueue. Yes, I disagree with the implementation of ...
Jan. 28, 2016, 10:15 a.m. (2016-01-28 10:15:35 UTC) #8
Eric
On 2016/01/26 23:28:43, sergei wrote: > I disagree with the implementation of ActiveQueue. You haven't ...
Feb. 3, 2016, 6:59 p.m. (2016-02-03 18:59:15 UTC) #9
Eric
Patch set 4 addresses Oleksandr's comments. It doesn't change the gyp file, pending discussion about ...
Feb. 4, 2016, 9:01 p.m. (2016-02-04 21:01:45 UTC) #10
Eric
July 27, 2016, 8:31 p.m. (2016-07-27 20:31:28 UTC) #11
Renamed description to add #1234. The old threading code for log writing is now
the only remaining use of CString in the code.

Powered by Google App Engine
This is Rietveld