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

Delta Between Two Patch Sets: src/plugin/PluginDebug.cpp

Issue 5187613258416128: Issue #1234 - Rewrite internals of debug facility (Closed)
Left Patch Set: capitalize a function name Created March 20, 2015, 10:09 a.m.
Right Patch Set: rebase only Created Nov. 26, 2015, 1:02 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « src/plugin/PluginDebug.h ('k') | src/plugin/PluginSettings.h » ('j') | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2015 Eyeo GmbH 3 * Copyright (C) 2006-2015 Eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12 * GNU General Public License for more details. 12 * GNU General Public License for more details.
13 * 13 *
14 * You should have received a copy of the GNU General Public License 14 * You should have received a copy of the GNU General Public License
15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. 15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
16 */ 16 */
17 17
18 #include "PluginStdAfx.h" 18 #include "PluginStdAfx.h"
19 #include "PluginDebug.h" 19 #include "PluginDebug.h"
20 #include "PluginClientBase.h"
20 #include "PluginMutex.h" 21 #include "PluginMutex.h"
21 #include "PluginClient.h" 22 #include "PluginClientBase.h"
22 #include "../shared/Utils.h" 23 #include "../shared/Utils.h"
23 #include <iomanip> 24 #include <iomanip>
25 #include <memory>
24 26
25 namespace 27 namespace
26 { 28 {
27 class CPluginDebugLock 29 class CPluginDebugLock
28 : public CPluginMutex 30 : public CPluginMutex
29 { 31 {
30 private: 32 private:
31 static CComAutoCriticalSection s_criticalSectionDebugLock; 33 static CComAutoCriticalSection s_criticalSectionDebugLock;
32 34
33 public: 35 public:
34 CPluginDebugLock() 36 CPluginDebugLock()
35 : CPluginMutex(L"DebugFile", PLUGIN_ERROR_MUTEX_DEBUG_FILE) 37 : CPluginMutex(L"DebugFile", PLUGIN_ERROR_MUTEX_DEBUG_FILE)
36 { 38 {
37 s_criticalSectionDebugLock.Lock(); 39 s_criticalSectionDebugLock.Lock();
38 } 40 }
39 41
40 ~CPluginDebugLock() 42 ~CPluginDebugLock()
41 { 43 {
42 s_criticalSectionDebugLock.Unlock(); 44 s_criticalSectionDebugLock.Unlock();
43 } 45 }
44 }; 46 };
45 47
46 CComAutoCriticalSection CPluginDebugLock::s_criticalSectionDebugLock; 48 CComAutoCriticalSection CPluginDebugLock::s_criticalSectionDebugLock;
47 49
48 class LogText 50 class LogText
49 { 51 {
50 public: 52 public:
51 virtual std::string Text() =0; 53 virtual std::string Text() const = 0;
sergei 2015/03/30 13:45:26 I guess, the method should have const modifier.
Eric 2015/03/31 15:42:22 Done.
54 virtual ~LogText() {};
52 }; 55 };
53 56
54 class LogTextFixed 57 class LogTextFixed
sergei 2015/12/02 13:53:00 What about naming of classes differently? Let's a
Eric 2015/12/02 15:01:24 No. Naming the subclasses of 'class LogText' with
55 : public LogText 58 : public LogText
56 { 59 {
57 protected: 60 protected:
58 const std::string fixedText; 61 const std::string fixedText;
59 62
60 public: 63 public:
61 LogTextFixed(std::string text) 64 explicit LogTextFixed(const std::string& text)
sergei 2015/03/30 13:45:26 explicit and const ref are missed
Eric 2015/03/31 15:42:22 We don't have 'explict' in VS 2012. 'const' done.
sergei 2015/04/02 07:41:12 Why do we not have explicit in VS 2012? It's a goo
Eric 2015/05/19 15:13:56 I'm confused. We don't have 'explicit' for convers
62 : fixedText(text) 65 : fixedText(text)
63 {} 66 {}
64 67
65 virtual std::string Text() override 68 explicit LogTextFixed(const std::exception& ex)
Eric 2015/03/20 10:17:04 I remembered to look this up. VS 2012 does support
69 : fixedText(std::string("!!! Exception: ") + ex.what())
70 {}
71
72 virtual std::string Text() const override
sergei 2015/11/30 14:15:03 I would propose to omit `virtual` when we use `ove
Eric 2015/11/30 17:39:01 It's good to have both in place. Makes the intent
sergei 2015/11/30 18:14:29 Actually, I would like to note that it's not relat
Eric 2015/12/01 20:43:58 OK.
66 { 73 {
67 return fixedText; 74 return fixedText;
68 } 75 }
69 }; 76 };
70 77
71 class LogTextErrorCode 78 class LogTextErrorCode
72 : public LogTextFixed 79 : public LogTextFixed
73 { 80 {
74 protected: 81 protected:
75 const DWORD errorCode; 82 const DWORD errorCode;
76 83
77 public: 84 public:
78 LogTextErrorCode(DWORD errorCode, const std::string& text) 85 LogTextErrorCode(DWORD errorCode, const std::string& text)
79 : LogTextFixed(text), errorCode(errorCode) 86 : LogTextFixed(text), errorCode(errorCode)
80 {} 87 {}
81 88
82 virtual std::string Text() override 89 virtual std::string Text() const override
83 { 90 {
84 std::ostringstream ss; 91 std::ostringstream ss;
85 ss << fixedText << ". error = " << errorCode << " (0x"; 92 ss << fixedText << ". error = " << errorCode << " (0x";
86 ss << std::setfill('0') << std::setw(2 * sizeof(DWORD)) << std::hex << err orCode; 93 ss << std::setfill('0') << std::setw(2 * sizeof(DWORD)) << std::hex << err orCode;
87 ss << ")"; 94 ss << ")";
88 return ss.str(); 95 return ss.str();
89 } 96 }
90 }; 97 };
91 98
92 class LogTextException
93 : public LogText
94 {
95 protected:
96 const std::exception ex;
97
98 public:
99 LogTextException(const std::exception& ex)
100 : ex(ex)
sergei 2015/03/30 13:45:26 I'm not sure that it is safe to copy std::exceptio
Eric 2015/03/31 15:42:22 The point behind subclassing LogText is to defer c
Eric 2015/03/31 15:42:22 You're right; it's not. There's also no 'clone()'
sergei 2015/04/02 07:41:12 It's clear, but I would say lazy evaluation here m
Eric 2015/05/19 15:13:56 I'll leave it in. It's not harming anything. I may
101 {}
102
103 virtual std::string Text() override
104 {
105 auto text = std::string("!!! Exception: ");
106 text += ex.what();
107 return text;
108 }
109 };
110
111 /** 99 /**
112 * Wrapper around SYSTEMTIME allows initialization of 'const' instances. 100 * Wrapper around SYSTEMTIME allows initialization of 'const' instances.
113 */ 101 */
114 struct SystemTime 102 struct SystemTime
115 : public SYSTEMTIME 103 : public SYSTEMTIME
116 { 104 {
117 SystemTime() 105 SystemTime()
118 { 106 {
119 ::GetSystemTime(static_cast<SYSTEMTIME*>(this)); 107 ::GetSystemTime(static_cast<SYSTEMTIME*>(this));
120 } 108 }
121 }; 109 };
122 110
123 class LogEntry 111 class LogEntry
124 { 112 {
125 const std::unique_ptr<LogText> text; 113 const std::unique_ptr<LogText> text;
sergei 2015/12/02 13:53:00 Just in case, what is the reason of extensively pu
Eric 2015/12/02 15:01:24 'const' members mean "initialized only at construc
126 114
127 std::string InitialPrefix() 115 std::string InitialPrefix() const
128 { 116 {
129 std::stringstream ss; 117 std::stringstream ss;
130 ss << std::setfill('0') << std::setw(2) << st.wHour; 118 ss << std::setfill('0') << std::setw(2) << st.wHour;
131 ss << ":"; 119 ss << ":";
132 ss << std::setfill('0') << std::setw(2) << st.wMinute; 120 ss << std::setfill('0') << std::setw(2) << st.wMinute;
133 ss << ":"; 121 ss << ":";
134 ss << std::setfill('0') << std::setw(2) << st.wSecond; 122 ss << std::setfill('0') << std::setw(2) << st.wSecond;
135 ss << "."; 123 ss << ".";
136 ss << std::setfill('0') << std::setw(3) << st.wMilliseconds; 124 ss << std::setfill('0') << std::setw(3) << st.wMilliseconds;
137 ss << " ["; 125 ss << " [";
138 ss << std::setfill(' ') << std::setw(5) << threadId; 126 ss << std::setfill(' ') << std::setw(5) << threadId;
139 ss << "] - "; 127 ss << "] - ";
140 return ss.str(); 128 return ss.str();
141 } 129 }
142 130
143 std::string SubsequentPrefix() 131 std::string SubsequentPrefix() const
144 { 132 {
145 return " + "; 133 return " + ";
146 } 134 }
147 135
148 public: 136 public:
149 /** 137 /**
150 * The time at which the log-generating statement executes. 138 * The time at which the log-generating statement executes.
151 */ 139 */
152 const SystemTime st; 140 const SystemTime st;
153 141
154 /** 142 /**
155 * The process within which the log-generating statement executes. 143 * The process within which the log-generating statement executes.
156 */ 144 */
157 const DWORD processId; 145 const DWORD processId;
158 146
159 /** 147 /**
160 * The thread within which the log-generating statement executes. 148 * The thread within which the log-generating statement executes.
161 */ 149 */
162 const DWORD threadId; 150 const DWORD threadId;
163 151
164 LogEntry(LogText* text) 152 explicit LogEntry(LogText* text)
sergei 2015/03/30 13:45:26 explicit is missed
Eric 2015/03/31 15:42:22 No 'explicit' in VS 2012.
165 : processId(::GetCurrentProcessId()), threadId(::GetCurrentThreadId()), te xt(text) 153 : processId(::GetCurrentProcessId()), threadId(::GetCurrentThreadId()), te xt(text)
166 {} 154 {}
167 155
168 LogEntry(LogText* text, DWORD processId, DWORD threadId) 156 LogEntry(LogText* text, DWORD processId, DWORD threadId)
169 : processId(processId), threadId(threadId), text(text) 157 : processId(processId), threadId(threadId), text(text)
170 {} 158 {}
171 159
172 void Write(std::ostream& out) 160 void Write(std::ostream& out) const
sergei 2015/03/30 13:45:26 this method should be const
Eric 2015/03/31 15:42:22 Done.
173 { 161 {
174 CPluginDebugLock lock; 162 CPluginDebugLock lock;
175 if (lock.IsLocked()) 163 if (lock.IsLocked())
176 { 164 {
177 auto lines = text->Text(); 165 auto lines = text->Text();
178 size_t linePosition = 0; 166 size_t linePosition = 0;
179 while (true) 167 while (true)
180 { 168 {
181 auto eolPosition = lines.find('\n', linePosition); 169 auto eolPosition = lines.find('\n', linePosition);
182 auto prefix = linePosition == 0 ? InitialPrefix() : SubsequentPrefix() ; 170 auto prefix = linePosition == 0 ? InitialPrefix() : SubsequentPrefix() ;
183 out << prefix; 171 out << prefix;
184 if (eolPosition == std::string::npos) 172 if (eolPosition == std::string::npos)
185 { 173 {
186 out << lines.substr(linePosition) << "\n"; 174 out << lines.substr(linePosition) << "\n";
187 break; 175 break;
188 } 176 }
189 else 177 else
190 { 178 {
191 out << lines.substr(linePosition, eolPosition - linePosition) << "\n "; 179 out << lines.substr(linePosition, eolPosition - linePosition) << "\n ";
192 linePosition = eolPosition + 1; 180 linePosition = eolPosition + 1;
193 } 181 }
194 } 182 }
195 out.flush(); 183 out.flush();
196 } 184 }
197 } 185 }
198 186
199 void WriteFile(const std::wstring& logFileName) 187 void WriteFile(const std::wstring& logFileName) const
sergei 2015/03/30 13:45:26 it also should be const
Eric 2015/03/31 15:42:22 Done.
200 { 188 {
201 std::ofstream out; 189 std::ofstream out;
202 out.open(logFileName, std::ios::app); 190 out.open(logFileName, std::ios::app);
sergei 2015/12/02 13:53:00 I wonder whether we need a synchronization here. E
Eric 2015/12/02 15:01:24 Easy answer: This locking behavior is the same as
203 Write(out); 191 Write(out);
204 } 192 }
205 }; 193 };
206 194
207 std::wstring GetDataPath(const std::wstring& filename) 195 std::wstring GetDataPath(const std::wstring& filename)
208 { 196 {
209 return GetAppDataPath() + L"\\" + filename; 197 return GetAppDataPath() + L"\\" + filename;
210 } 198 }
211 199
212 void LogWriteDefault(LogEntry& le) 200 void LogWriteDefault(const LogEntry& le)
sergei 2015/03/30 13:45:26 LogEntry should be const ref
Eric 2015/03/31 15:42:22 Done.
213 { 201 {
214 std::wstring debugFileName = GetDataPath(L"debug_" + std::to_wstring(le.proc essId) + L".txt"); 202 std::wstring debugFileName = GetDataPath(L"debug_" + std::to_wstring(le.proc essId) + L".txt");
215 le.WriteFile(debugFileName); 203 le.WriteFile(debugFileName);
216 } 204 }
217 205
218 void LogWriteResult(LogEntry& le) 206 void LogWriteResult(const LogEntry& le)
sergei 2015/03/30 13:45:26 it also should be const
Eric 2015/03/31 15:42:22 Done.
219 { 207 {
220 std::wstring debugFileName = GetDataPath(L"debug_result.txt"); 208 std::wstring debugFileName = GetDataPath(L"debug_result.txt");
221 le.WriteFile(debugFileName); 209 le.WriteFile(debugFileName);
222 } 210 }
223 } 211 }
224 212
225 #ifdef ENABLE_DEBUG_INFO 213 #ifdef ENABLE_DEBUG_INFO
226 214
227 void CPluginDebug::Debug(const std::string& text) 215 void CPluginDebug::Debug(const std::string& text)
228 { 216 {
(...skipping 10 matching lines...) Expand all
239 void CPluginDebug::DebugSystemException(const std::system_error& ex, int errorId , int errorSubid, const std::string& description) 227 void CPluginDebug::DebugSystemException(const std::system_error& ex, int errorId , int errorSubid, const std::string& description)
240 { 228 {
241 std::string message = description + ", " + ex.code().message() + ", " + ex.wha t(); 229 std::string message = description + ", " + ex.code().message() + ", " + ex.wha t();
242 DEBUG_ERROR_LOG(ex.code().value(), errorId, errorSubid, message); 230 DEBUG_ERROR_LOG(ex.code().value(), errorId, errorSubid, message);
243 } 231 }
244 232
245 #if (defined ENABLE_DEBUG_INFO) 233 #if (defined ENABLE_DEBUG_INFO)
246 234
247 void CPluginDebug::DebugException(const std::exception& ex) 235 void CPluginDebug::DebugException(const std::exception& ex)
248 { 236 {
249 auto lt = new LogTextException(ex); 237 auto lt = new LogTextFixed(ex);
238 LogEntry le(lt);
250 #ifdef ENABLE_DEBUG_ERROR 239 #ifdef ENABLE_DEBUG_ERROR
251 LogWriteDefault(LogEntry(lt)); 240 LogWriteDefault(le);
252 #endif 241 #endif
253 DEBUG_SELFTEST( 242 DEBUG_SELFTEST(
254 "*************************************************************************** *****\n" 243 "*************************************************************************** *****\n"
255 + lt.text() + "\n" 244 + lt->text() + "\n"
256 "*************************************************************************** *****") 245 "*************************************************************************** *****")
257 } 246 }
258 247
259 void CPluginDebug::DebugErrorCode(DWORD errorCode, const std::string& error, DWO RD processId, DWORD threadId) 248 void CPluginDebug::DebugErrorCode(DWORD errorCode, const std::string& error, DWO RD processId, DWORD threadId)
260 { 249 {
261 auto lt = new LogTextErrorCode(errorCode, error); 250 auto lt = new LogTextErrorCode(errorCode, error);
251 LogEntry le(lt, processId, threadId);
262 #ifdef ENABLE_DEBUG_ERROR 252 #ifdef ENABLE_DEBUG_ERROR
263 LogWriteDefault(LogEntry(lt, processId, threadId)); 253 LogWriteDefault(le);
264 #endif 254 #endif
265 DEBUG_SELFTEST( 255 DEBUG_SELFTEST(
266 "*************************************************************************** *****\n" 256 "*************************************************************************** *****\n"
267 + lt.text() + "\n" 257 + lt->text() + "\n"
268 "*************************************************************************** *****") 258 "*************************************************************************** *****")
269 } 259 }
270 260
271 #endif 261 #endif
272 262
273 // ============================================================================ 263 // ============================================================================
274 // Debug result 264 // Debug result
275 // ============================================================================ 265 // ============================================================================
276 266
277 #ifdef ENABLE_DEBUG_RESULT 267 #ifdef ENABLE_DEBUG_RESULT
(...skipping 49 matching lines...) Expand 10 before | Expand all | Expand 10 after
327 317
328 318
329 #ifdef ENABLE_DEBUG_RESULT_IGNORED 319 #ifdef ENABLE_DEBUG_RESULT_IGNORED
330 320
331 void CPluginDebug::DebugResultIgnoring(const std::wstring& type, const std::wstr ing& src, const std::wstring& domain) 321 void CPluginDebug::DebugResultIgnoring(const std::wstring& type, const std::wstr ing& src, const std::wstring& domain)
332 { 322 {
333 DebugResultFormat(L"Ignored", type, domain.empty() ? L"-" : domain, Shorten(sr c)); 323 DebugResultFormat(L"Ignored", type, domain.empty() ? L"-" : domain, Shorten(sr c));
334 } 324 }
335 325
336 #endif // ENABLE_DEBUG_RESULT_IGNORED 326 #endif // ENABLE_DEBUG_RESULT_IGNORED
LEFTRIGHT

Powered by Google App Engine
This is Rietveld