Hello steveblock, michaeln,
I'd like you to do a code review. Please execute
g4 diff -c 8175346
or point your web browser to
http://mondrian/8175346
to review the following code:
Change 8175346 by [EMAIL PROTECTED] on 2008/09/04 14:56:18 *pending*
Remove logging statements from HttpHandler.
R=steveblock,michaeln
[EMAIL PROTECTED]
DELTA=99 (0 added, 91 deleted, 8 changed)
OCL=8175346
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/ie/http_handler_ie.cc#3
edit
99 delta lines: 0 added, 91 deleted, 8 changed
Also consider running:
g4 lint -c 8175346
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 8175346 by [EMAIL PROTECTED] on 2008/09/04 14:56:18 *pending*
Remove logging statements from HttpHandler.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/ie/http_handler_ie.cc#3
edit
====
//depot/googleclient/gears/opensource/gears/localserver/ie/http_handler_ie.cc#3
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/localserver/ie/http_handler_ie.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/localserver/ie/http_handler_ie.cc
2008-09-04 14:56:27.000000000 +1000
+++ googleclient/gears/opensource/gears/localserver/ie/http_handler_ie.cc
2008-09-04 14:53:02.000000000 +1000
@@ -172,7 +172,6 @@
return hr;
}
#endif
- LOG16((L"HttpHandler::Registered\n"));
return hr;
}
@@ -204,7 +203,6 @@
session->SetSessionOption(kBypassIInternetProtocolInfoOption,
&bypass, sizeof(BOOL), 0);
#endif
- LOG16((L"HttpHandler::Unregistered\n"));
return S_OK;
}
@@ -251,12 +249,6 @@
if (!http_handler_->is_passingthru_)
return E_FAIL;
-#ifdef DEBUG
- LOG16((L"PassthruSink::ReportProgress( %s, %s )\n",
- GetBindStatusLabel(status_code),
- status_text ? status_text : L""));
-#endif
-
if (status_code == BINDSTATUS_REDIRECTING) {
// status_text contains the redirect url in this case
WebCacheDB* db = WebCacheDB::GetDB();
@@ -266,7 +258,6 @@
// causes URMLON to abandon this handler instance an create a new one
// to follow the redirect. When that new instance of our handler is
// created, it will satisfy the request locally.
- LOG16((L"PassthruSink::ReportProgress - hijacking redirect\n"));
return BaseClass::ReportResult(INET_E_REDIRECT_FAILED,
HttpConstants::HTTP_FOUND,
status_text);
@@ -367,7 +358,6 @@
}
HttpHandler::~HttpHandler() {
- LOG16((L"~HttpHandler\n"));
g_active_handlers.Remove(this); // okay to Remove() multiple times from set
}
@@ -389,7 +379,6 @@
if (FAILED(rv)) {
return rv;
}
- LOG16((L"HttpHandler::StartEx( %s )\n", uri_bstr.m_str));
rv = StartImpl(uri_bstr.m_str, protocol_sink, bind_info, flags, reserved);
if (rv == INET_E_USE_DEFAULT_PROTOCOLHANDLER) {
protocol_sink_.Release();
@@ -411,7 +400,6 @@
/* [in] */ IInternetBindInfo *bind_info,
/* [in] */ DWORD flags,
/* [in] */ HANDLE_PTR reserved) {
- LOG16((L"HttpHandler::Start( %s )\n", url));
HRESULT rv = StartImpl(url, protocol_sink, bind_info, flags, reserved);
if (rv == INET_E_USE_DEFAULT_PROTOCOLHANDLER) {
protocol_sink_.Release();
@@ -428,7 +416,6 @@
STDMETHODIMP HttpHandler::Continue(
/* [in] */ PROTOCOLDATA *data) {
- LOG16((L"HttpHandler::Continue(data)\n"));
if (is_passingthru_)
return BaseClass::Continue(data);
else if (is_handling_)
@@ -440,7 +427,6 @@
STDMETHODIMP HttpHandler::Abort(
/* [in] */ HRESULT reason,
/* [in] */ DWORD options) {
- LOG16((L"HttpHandler::Abort()\n"));
if (is_passingthru_) {
return BaseClass::Abort(reason, options);
} else if (is_handling_) {
@@ -456,7 +442,6 @@
STDMETHODIMP HttpHandler::Terminate(
/* [in] */ DWORD options) {
- LOG16((L"HttpHandler::Terminate()\n"));
protocol_sink_.Release();
http_negotiate_.Release();
g_active_handlers.Remove(this);
@@ -471,7 +456,6 @@
}
STDMETHODIMP HttpHandler::Suspend() {
- LOG16((L"HttpHandler::Suspend()\n"));
if (is_passingthru_)
return BaseClass::Suspend();
else if (is_handling_)
@@ -481,7 +465,6 @@
}
STDMETHODIMP HttpHandler::Resume() {
- LOG16((L"HttpHandler::Resume()\n"));
if (is_passingthru_)
return BaseClass::Resume();
else if (is_handling_)
@@ -497,12 +480,10 @@
/* [out] */ ULONG *pcbRead) {
if (is_passingthru_) {
HRESULT hr = BaseClass::Read(pv, cb, pcbRead);
- LOG16((L"HttpHandler::Read() - passing thru, %d bytes\n", *pcbRead));
return hr;
} else if (is_handling_) {
return ReadImpl(pv, cb, pcbRead);
} else {
- LOG16((L"HttpHandler::Read() - unexpected\n"));
return E_UNEXPECTED;
}
}
@@ -511,7 +492,6 @@
/* [in] */ LARGE_INTEGER dlibMove,
/* [in] */ DWORD dwOrigin,
/* [out] */ ULARGE_INTEGER *plibNewPosition) {
- LOG16((L"HttpHandler::Seek()\n"));
if (is_passingthru_)
return BaseClass::Seek(dlibMove, dwOrigin, plibNewPosition);
else if (is_handling_)
@@ -522,7 +502,6 @@
STDMETHODIMP HttpHandler::LockRequest(
/* [in] */ DWORD dwOptions) {
- LOG16((L"HttpHandler::LockRequest()\n"));
if (is_passingthru_)
return BaseClass::LockRequest(dwOptions);
else if (is_handling_)
@@ -532,7 +511,6 @@
}
STDMETHODIMP HttpHandler::UnlockRequest() {
- LOG16((L"HttpHandler::UnlockRequest()\n"));
if (is_passingthru_)
return BaseClass::UnlockRequest();
else if (is_handling_)
@@ -552,7 +530,6 @@
/* [in] */ DWORD cchResult,
/* [out] */ DWORD *pcchResult,
/* [in] */ DWORD dwReserved) {
- LOG16((L"HttpHandler::ParseUrl()\n"));
if (is_passingthru_)
return BaseClass::ParseUrl(pwzUrl, ParseAction, dwParseFlags,
pwzResult, cchResult, pcchResult,
@@ -571,7 +548,6 @@
/* [in] */ DWORD cchResult,
/* [out] */ DWORD *pcchResult,
/* [in] */ DWORD dwReserved) {
- LOG16((L"HttpHandler::CombineUrl()\n"));
if (is_passingthru_)
return BaseClass::CombineUrl(pwzBaseUrl, pwzRelativeUrl,
dwCombineFlags, pwzResult,
@@ -586,7 +562,6 @@
/* [in] */ LPCWSTR pwzUrl1,
/* [in] */ LPCWSTR pwzUrl2,
/* [in] */ DWORD dwCompareFlags) {
- LOG16((L"HttpHandler::CompareUrl()\n"));
if (is_passingthru_)
return BaseClass::CompareUrl(pwzUrl1, pwzUrl2, dwCompareFlags);
else
@@ -601,7 +576,6 @@
/* [in] */ DWORD cbBuffer,
/* [in, out] */ DWORD *pcbBuf,
/* [in] */ DWORD dwReserved) {
- LOG16((L"HttpHandler::QueryInfo()\n"));
if (is_passingthru_)
return BaseClass::QueryInfo(pwzUrl, QueryOption, dwQueryFlags,
pBuffer, cbBuffer, pcbBuf,
@@ -619,13 +593,11 @@
// will return the value expected by the caller.
STDMETHODIMP HttpHandler::SetPriority(
/* [in] */ LONG nPriority) {
- LOG16((L"HttpHandler::SetPriority()\n"));
return BaseClass::SetPriority(nPriority);
}
STDMETHODIMP HttpHandler::GetPriority(
/* [out] */ LONG *pnPriority) {
- LOG16((L"HttpHandler::GetPriority()\n"));
return BaseClass::GetPriority(pnPriority);
}
@@ -633,7 +605,6 @@
// TODO(michaeln): our handler implements this interface for the purpose
// of passing thru only, when not in passthru mode what should we do?
STDMETHODIMP HttpHandler::Prepare() {
- LOG16((L"HttpHandler::Prepare()\n"));
if (is_passingthru_)
return BaseClass::Prepare();
else if (is_handling_)
@@ -643,7 +614,6 @@
}
STDMETHODIMP HttpHandler::Continue() {
- LOG16((L"HttpHandler::Continue()\n"));
if (is_passingthru_)
return BaseClass::Continue();
else if (is_handling_)
@@ -657,7 +627,6 @@
/* [in] */ DWORD option,
/* [in, out] */ LPVOID buffer,
/* [in, out] */ DWORD *len) {
- LOG16((L"HttpHandler::QueryOption(%d)\n", option));
if (is_passingthru_)
return BaseClass::QueryOption(option, buffer, len);
else if (is_handling_)
@@ -673,7 +642,6 @@
/* [in, out] */ DWORD *len,
/* [in, out] */ DWORD *flags,
/* [in, out] */ DWORD *reserved) {
- LOG16((L"HttpHandler::QueryInfo(%d)\n", option));
if (is_passingthru_)
return BaseClass::QueryInfo(option, buffer, len, flags, reserved);
else if (is_handling_)
@@ -691,7 +659,6 @@
/* [out][in] */ DWORD *pcbCacheFile,
/* [out][in] */ DWORD *pdwWinInetError,
/* [out][in] */ DWORD *pdwReserved) {
- LOG16((L"HttpHandler::SetCacheExtension()\n"));
if (is_passingthru_)
return BaseClass::SetCacheExtension(pwzExt, pszCacheFile, pcbCacheFile,
pdwWinInetError, pdwReserved);
@@ -710,7 +677,6 @@
/* [out][in] */ DWORD *pcchCacheFile,
/* [out] */ DWORD *pdwWinInetError,
/* [out] */ DWORD *pdwReserved) {
- LOG16((L"HttpHandler::SetCacheExtension2()\n"));
if (is_passingthru_)
return BaseClass::SetCacheExtension2(pwzExt, pwzCacheFile, pcchCacheFile,
pdwWinInetError, pdwReserved);
@@ -728,24 +694,14 @@
LPCWSTR status_text) {
if (was_terminated_ || was_aborted_ || !protocol_sink_)
return E_UNEXPECTED;
-#ifdef DEBUG
- LOG16((L"Calling ReportProgress( %s, %s )\n",
GetBindStatusLabel(status_code),
- status_text ? status_text : L""));
-#endif
- HRESULT hr = protocol_sink_->ReportProgress(status_code, status_text);
- if (FAILED(hr))
- LOG16((L" protocol_sink->ReportProgress error = %d\n", hr));
- return hr;
+ return protocol_sink_->ReportProgress(status_code, status_text);
}
HRESULT HttpHandler::CallReportData(DWORD flags, ULONG progress,
ULONG progress_max) {
if (was_terminated_ || was_aborted_ || !protocol_sink_)
return E_UNEXPECTED;
- HRESULT hr = protocol_sink_->ReportData(flags, progress, progress_max);
- if (FAILED(hr))
- LOG16((L" protocol_sink->ReportData error = %d\n", hr));
- return hr;
+ return protocol_sink_->ReportData(flags, progress, progress_max);
}
HRESULT HttpHandler::CallReportResult(HRESULT result,
@@ -754,10 +710,7 @@
if (was_terminated_ || has_reported_result_ || !protocol_sink_)
return E_UNEXPECTED;
has_reported_result_ = true;
- HRESULT hr = protocol_sink_->ReportResult(result, error, result_text);
- if (FAILED(hr))
- LOG16((L" protocol_sink->ReportResult error = %d\n", hr));
- return hr;
+ return protocol_sink_->ReportResult(result, error, result_text);
}
HRESULT HttpHandler::CallBeginningTransaction(LPCWSTR url,
@@ -766,11 +719,8 @@
LPWSTR *additional_headers) {
if (was_terminated_ || was_aborted_ || !http_negotiate_)
return E_UNEXPECTED;
- HRESULT hr = http_negotiate_->BeginningTransaction(url, headers, reserved,
- additional_headers);
- if (FAILED(hr))
- LOG16((L" http_negotiate->BeginningTransaction error = %d\n", hr));
- return hr;
+ return http_negotiate_->BeginningTransaction(url, headers, reserved,
+ additional_headers);
}
HRESULT HttpHandler::CallOnResponse(DWORD status_code,
@@ -779,12 +729,9 @@
LPWSTR *additional_request_headers) {
if (was_terminated_ || was_aborted_ || !http_negotiate_)
return E_UNEXPECTED;
- HRESULT hr = http_negotiate_->OnResponse(status_code, response_headers,
- request_headers,
- additional_request_headers);
- if (FAILED(hr))
- LOG16((L" http_negotiate->OnResponse error = %d\n", hr));
- return hr;
+ return http_negotiate_->OnResponse(status_code, response_headers,
+ request_headers,
+ additional_request_headers);
}
@@ -824,7 +771,6 @@
bindinfo.cbSize = sizeof(bindinfo);
HRESULT hr = bind_info->GetBindInfo(&bindinfoflags, &bindinfo);
if (FAILED(hr)) {
- LOG16((L"GetBindInfo failed, error = %d\n", hr));
return hr;
}
@@ -834,9 +780,6 @@
// IWinInetHttpInfo::QueryInfo return when invoked from within the caller's
// implementation of IHttpNegotiate::BeginningTransaction?
- LOG16((L"HttpHandler::StartImpl( %s ) "
- L"flags=0x%x, bindinfoflags=0x%x, dwOptions=0x%x\n",
- url, flags, bindinfoflags, bindinfo.dwOptions));
#ifdef DEBUG
TraceBindFlags(bindinfoflags);
#endif
@@ -848,11 +791,8 @@
if (bindinfo.dwBindVerb != BINDVERB_GET) {
if ((bindinfo.dwBindVerb == BINDVERB_CUSTOM) &&
(wcscmp(HttpConstants::kHttpHEAD, bindinfo.szCustomVerb) == 0)) {
- LOG16((L" HEAD request"));
is_head_request_ = true;
} else {
- LOG16((L" not a GET or HEAD request - "
- L"passing thru to default handler\n"));
is_passingthru_ = true;
return INET_E_USE_DEFAULT_PROTOCOLHANDLER;
}
@@ -868,7 +808,6 @@
IID_QueryBypassCache,
reinterpret_cast<void**>(¬_used));
if (bypass_cache) {
- LOG16((L" by passing cache - using default protocol handler\n"));
return INET_E_USE_DEFAULT_PROTOCOLHANDLER;
}
}
@@ -881,7 +820,6 @@
bool is_captured = db->Service(url, NULL, is_head_request_, &payload_);
if (!is_captured) {
- LOG16((L" cache miss - passing thru to default protocol handler\n"));
if (kAlertCacheMiss && !ActiveXUtils::IsOnline()) {
MessageBoxW(NULL, url, L"WebCapture cache miss",
MB_OK | MB_SETFOREGROUND | MB_TOPMOST);
@@ -904,7 +842,6 @@
int num_redirects = 0;
while (payload_.IsHttpRedirect()) {
if (!payload_.GetHeader(HttpConstants::kLocationHeader, &redirect_url)) {
- LOG16((L" redirect with no location - using default handler\n"));
is_passingthru_ = true;
return INET_E_USE_DEFAULT_PROTOCOLHANDLER;
}
@@ -913,8 +850,6 @@
// We don't have a response for redirect_url. So report
// INET_E_REDIRECT_FAILED which causes the aggregating outer
// object (URLMON) to create a new inner APP and release us.
- LOG16((L" cache hit, almost - redirect out of cache ( %s )\n",
- redirect_url.c_str()));
is_handling_ = true; // set to true so we respond to QueryInfo calls
return CallReportResult(INET_E_REDIRECT_FAILED,
HttpConstants::HTTP_FOUND,
@@ -924,14 +859,9 @@
// Make sure we don't get stuck in an infinite redirect loop.
++num_redirects;
if (num_redirects > HttpConstants::kMaxRedirects) {
- LOG16((STRING16(L"Redirect chain too long %s -> %s"),
- url, redirect_url.c_str()));
return E_FAIL;
}
}
- LOG16((L" cache hit - redirect ( %s )\n", redirect_url.c_str()));
- } else {
- LOG16((L" cache hit\n"));
}
// Ok, we're going to satisfy this request from our cache
@@ -1022,7 +952,6 @@
hr = CallReportResult(S_OK, payload_.status_code, status_text.c_str());
if (FAILED(hr)) return hr;
- LOG16((L"HttpHandler::StartImpl( %s, %d ): YES\n", url, response_size));
return S_OK;
}
@@ -1033,7 +962,6 @@
HRESULT HttpHandler::ReadImpl(void *buffer,
ULONG byte_count,
ULONG *bytes_read) {
- LOG16((L"HttpHandler::ReadImpl(%d)\n", byte_count));
if (is_handling_) {
std::vector<uint8> *data = payload_.data.get();
size_t bytes_available = data ? (data->size() - read_pointer_) : 0;
@@ -1049,13 +977,11 @@
}
if (bytes_available - bytes_to_copy == 0) {
- LOG16((L"----> HttpHandler::ReadImpl() complete\n"));
return S_FALSE;
} else {
return S_OK;
}
} else {
- LOG16((L"----> HttpHandler::ReadImpl() E_UNEXPECTED\n"));
assert(false);
return E_UNEXPECTED;
}
@@ -1069,11 +995,6 @@
HRESULT HttpHandler::QueryOptionImpl(/* [in] */ DWORD dwOption,
/* [in, out] */ LPVOID pBuffer,
/* [in, out] */ DWORD *pcbBuf) {
-#ifdef DEBUG
- LOG16((L"HttpHandler::QueryOption(%s (%d))\n",
- GetWinInetInfoLabel(dwOption), dwOption));
-#endif
-
if (!is_handling_) {
return E_UNEXPECTED;
}
@@ -1120,13 +1041,6 @@
bool flag_number = (flags & HTTP_QUERY_FLAG_NUMBER) != 0;
bool flag_coalesce = (flags & HTTP_QUERY_FLAG_COALESCE) != 0;
-#ifdef DEBUG
- LOG16((L"HttpHandler::QueryInfo(%s (%d), %d)\n",
- GetWinInetHttpInfoLabel(dwOption),
- dwOption,
- pdwFlags ? *pdwFlags : -1));
-#endif
-
if (!is_handling_) {
return E_UNEXPECTED;
}
@@ -1366,11 +1280,6 @@
return INET_E_DEFAULT_ACTION;
}
-#ifdef DEBUG
- LOG16((L"HttpHandler::IInternetProtocolInfo::QueryInfo(%s (%d), %s)\n",
- GetProtocolInfoLabel(queryOption), queryOption, pwzUrl));
-#endif
-
return ReturnBoolean(result, pBuffer, cbBuffer, pcbBuf);
}
