Cleanup

src/plugin/AdblockPlusClient.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginFilter.h"
 #include "PluginClientFactory.h"
 #include "PluginMutex.h"
 #include "PluginClass.h"
 
 #include "AdblockPlusClient.h"
@@ skipping 85 matching lines @@
       description.title = ToUtf16String(title);
       std::string specialization;
       message >> specialization;
       description.specialization = ToUtf16String(specialization);
       message >> description.listed;
       result.push_back(description);
     }
     return result;
   }
 
-  Communication::InputBuffer CallAdblockPlusEngineProcedure(Communication::OutputBuffer& message)
-  {
-    std::auto_ptr<Communication::Pipe> pipe = OpenAdblockPlusEnginePipe();
-    pipe->WriteMessage(message);
-    return pipe->ReadMessage();
-  }
-
-  Communication::InputBuffer CallAdblockPlusEngineProcedure(Communication::ProcType proc)
+  bool CallEngine(Communication::OutputBuffer& message, Communication::InputBuffer& inputBuffer = Communication::InputBuffer())
+  {
+    try
+    {
+      std::auto_ptr<Communication::Pipe> pipe = OpenAdblockPlusEnginePipe();
+      pipe->WriteMessage(message);
+      inputBuffer = pipe->ReadMessage();
+    }
+    catch (const std::exception& e)
+    {
+      DEBUG_GENERAL(e.what());
+      return false;
+    }
+    return true;
+  }
+
+  bool CallEngine(Communication::ProcType proc, Communication::InputBuffer& inputBuffer = Communication::InputBuffer())
   {
     Communication::OutputBuffer message;
     message << proc;
-    return CallAdblockPlusEngineProcedure(message);
+    return CallEngine(message, inputBuffer);
   }
 }
 
 CAdblockPlusClient* CAdblockPlusClient::s_instance = NULL;
 
 CAdblockPlusClient::CAdblockPlusClient() : CPluginClientBase()
 {
   m_filter = std::auto_ptr<CPluginFilter>(new CPluginFilter());
 }
 
@@ skipping 77 matching lines @@
   }
   m_criticalSectionFilter.Unlock();
   return isHidden;
 }
 
 bool CAdblockPlusClient::IsWhitelistedUrl(const std::wstring& url)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_IS_WHITELISTED_URL << ToUtf8String(url);
 
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(request);
-
-    bool isWhitelisted;
-    response >> isWhitelisted;
-    return isWhitelisted;
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
+  Communication::InputBuffer response;
+  if (!CallEngine(request, response)) 
     return false;
-  }
+
+  bool isWhitelisted;
+  response >> isWhitelisted;
+  return isWhitelisted;
 }
 
 int CAdblockPlusClient::GetIEVersion()
 {
   //HKEY_LOCAL_MACHINE\Software\Microsoft\Internet Explorer
   HKEY hKey;
   LSTATUS status = RegOpenKey(HKEY_LOCAL_MACHINE, L"Software\\Microsoft\\Internet Explorer", &hKey);
   if (status != 0)
   {
     return 0;
   }
   DWORD type, cbData;
   BYTE version[50];
   cbData = 50;
   status = RegQueryValueEx(hKey, L"Version", NULL, &type, (BYTE*)version, &cbData);
   if (status != 0)
   {
     return 0;
   }
   RegCloseKey(hKey);
   return (int)(version[0] - 48);
 }
 
 bool CAdblockPlusClient::Matches(const std::wstring& url, const std::wstring& contentType, const std::wstring& domain)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_MATCHES << ToUtf8String(url) << ToUtf8String(contentType) << ToUtf8String(domain);
 
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(request);
-
-    bool match;
-    response >> match;
-    return match;
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
+  Communication::InputBuffer response;
+  if (!CallEngine(request, response)) 
     return false;
-  }
+
+  bool match;
+  response >> match;
+  return match;
 }
 
 std::vector<std::wstring> CAdblockPlusClient::GetElementHidingSelectors(const std::wstring& domain)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_ELEMHIDE_SELECTORS << ToUtf8String(domain);
 
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(request);
-    return ReadStrings(response);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
+  Communication::InputBuffer response;
+  if (!CallEngine(request, response)) 
     return std::vector<std::wstring>();
-  }
+  return ReadStrings(response);
 }
 
 std::vector<SubscriptionDescription> CAdblockPlusClient::FetchAvailableSubscriptions()
 {
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(Communication::PROC_AVAILABLE_SUBSCRIPTIONS);
-    return ReadSubscriptions(response);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
+  Communication::InputBuffer response;
+  if (!CallEngine(Communication::PROC_AVAILABLE_SUBSCRIPTIONS, response)) 
     return std::vector<SubscriptionDescription>();
-  }
+  return ReadSubscriptions(response);
 }
 
 std::vector<SubscriptionDescription> CAdblockPlusClient::GetListedSubscriptions()
 {
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(Communication::PROC_LISTED_SUBSCRIPTIONS);
-    return ReadSubscriptions(response);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
+  Communication::InputBuffer response;
+  if (!CallEngine(Communication::PROC_LISTED_SUBSCRIPTIONS, response)) 
     return std::vector<SubscriptionDescription>();
-  }
+  return ReadSubscriptions(response);
 }
 
 void CAdblockPlusClient::SetSubscription(const std::wstring& url)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_SET_SUBSCRIPTION << ToUtf8String(url);
-
-  try
-  {
-    CallAdblockPlusEngineProcedure(request);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
-  }
+  CallEngine(request);
 }
 
 void CAdblockPlusClient::UpdateAllSubscriptions()
 {
-  try
-  {
-    CallAdblockPlusEngineProcedure(Communication::PROC_UPDATE_ALL_SUBSCRIPTIONS);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
-  }
+  CallEngine(Communication::PROC_UPDATE_ALL_SUBSCRIPTIONS);
 }
 
 std::vector<std::wstring> CAdblockPlusClient::GetExceptionDomains()
 {
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(Communication::PROC_GET_EXCEPTION_DOMAINS);
-    return ReadStrings(response);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
+  Communication::InputBuffer response;
+  if (!CallEngine(Communication::PROC_GET_EXCEPTION_DOMAINS)) 
     return std::vector<std::wstring>();
-  }
-}
-
-// A helper method to reuse the code
-void CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)

[Wladimir Palant, 2013/07/22 06:43:11]

I can't say I like the PostRequest() and FetchResponse() helpers:

1) Its purpose is non-obvious from the name. It is really
CallAdblockPlusEngineProcedure() but merely with exceptions swallowed.

2) Even if the purpose were clear, IMHO this makes the code harder to follow
because the exception handling is hidden. Instead of catching exceptions the
calling functions need to check whether they got a valid result.

So I would pretty much suggest removing these helpers. Feel free to ask Felix
for a third opinion if you disagree.

Note that you can reduce code size regardless if you use templates for the
GetPref/SetPref methods.

[Oleksandr, 2013/07/22 09:53:31]

I do disagree, at least partially. The code looks cleaner and more readable this
way IMHO. At least the PostRequest stuff seems to be worth it. Removed the
FetchResponse. Felix?
On 2013/07/22 06:43:11, Wladimir Palant wrote:

[Felix Dahlke, 2013/07/23 10:26:12]

I can see how you'd like to avoid the duplication Oleksandr, but I'm not a fan
of this either.

It looks like we're just swallowing all the exceptions anyway, so what we could
do is change CallAdblockPlusEngine so that it works like FetchResponse does now:
Return some special value on error instead of throwing an exception. What do you
think, Wladimir?

[Wladimir Palant, 2013/07/23 12:18:27]

If you look closely, we aren't really swallowing exceptions - depending on the
caller a different default behavior is in order. So FetchResponse() merely
replaces real exceptions with sort of exceptions based on return value: the
caller needs to check the return value and then go to the "default" execution
branch. There is a readability loss but IMHO no advantage whatsoever (worse: if
we forget to check the return value we might not even crash but produce wrong
results and cause issues that will be hard to investigate).

Arguably, PostRequest() has some value for the cases where we aren't interested
in the response. But then the function name needs to make the intention more
clear: CallEngineIgnoreResponse() if you like (and I would really shorten the
name of CallAdblockPlusEngineProcedure() into CallEngine()).

[Felix Dahlke, 2013/07/23 12:40:15]

What I meant was that we ignore the actual exception, we just log it in any
case. So I think we should log it in CallEngine(), not in the caller.

Then we have two options for handling errors: Use exceptions or return codes.
With exceptions it would look like this:

When we care:

try
{
  response = CallEngine(request);
}
catch (const std::exception&)
{
  return false;
}

When we don't care:

try
{
  CallEngine(request);
}
catch (const std::exception&) {}

With return codes, it could look like this:

When we care:

if (!CallEngine(request, response))
  return false;

When we don't care:

CallEngine(request);

I generally like exceptions, obviously, but I think in this case it's better to
go with the return code. We don't care what the exception is, and if my
suggestion is implemented, it's already being handled in CallEngine.

As you can see, I'm all for renaming the method to CallEngine() :)

-{
-  try
-  {
-    CallAdblockPlusEngineProcedure(request);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
-  }
+  return ReadStrings(response);
 }
 
 void CAdblockPlusClient::AddFilter(const std::wstring& text)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_ADD_FILTER << ToUtf8String(text);
-  PostRequest(request);
+  CallEngine(request);
 }
 
 void CAdblockPlusClient::RemoveFilter(const std::wstring& text)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_REMOVE_FILTER << ToUtf8String(text);
-  PostRequest(request);
+  CallEngine(request);
 }
 
 void CAdblockPlusClient::SetPref(const std::wstring& name, const std::wstring& value)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_SET_PREF << ToUtf8String(name) << ToUtf8String(value);
-  PostRequest(request);
-}
-
-void CAdblockPlusClient::SetPref(const std::string& name, const std::string& value)

[Wladimir Palant, 2013/07/22 06:43:11]

I don't think we need that function - the plugin always works with UTF-16, never
with UTF-8.

-{
-  Communication::OutputBuffer request;
-  request << Communication::PROC_SET_PREF << name << value;
-  PostRequest(request);
-}
-
+  CallEngine(request);
+}
 
 void CAdblockPlusClient::SetPref(const std::wstring& name, const int64_t & value)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_SET_PREF << ToUtf8String(name) << value;
-  PostRequest(request);
+  CallEngine(request);
 }
 
 void CAdblockPlusClient::SetPref(const std::wstring& name, bool value)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_SET_PREF << ToUtf8String(name) << value;
-  PostRequest(request);
-}
-
-Communication::InputBuffer CAdblockPlusClient::FetchResponse(const std::wstring& name)
+  CallEngine(request);
+}
+
+std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, const wchar_t* defaultValue)
+{
+  return GetPref(name, std::wstring(defaultValue));
+}
+std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, const std::wstring& defaultValue)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_PREF << ToUtf8String(name);
 
-  try
-  {
-    Communication::InputBuffer response = CallAdblockPlusEngineProcedure(request);
-    return response;
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
-  }
-  return Communication::InputBuffer("");
-}
-std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, LPCWSTR defaultValue)

[Wladimir Palant, 2013/07/22 06:43:11]

I guess you have that here because const char* is implicitly converted to bool
instead of const std::wstring&? Then please actually use const char* as type
instead of LPCWSTR here, I don't see why we should use the Windows-specific
type.

-{
-  return GetPref(name, std::wstring(defaultValue));
-}
-std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, const std::wstring& defaultValue)
-{
-  Communication::InputBuffer response = FetchResponse(name);
-  if (response.IsEmpty())
+  Communication::InputBuffer response;
+  if (!CallEngine(request, response)) 
     return defaultValue;
   bool success;
   response >> success;
   if (success)
   {
     std::string value;
     response >> value;
     return ToUtf16String(value);
   }
   else
     return defaultValue;
 }
 
 bool CAdblockPlusClient::GetPref(const std::wstring& name, bool defaultValue)
 {
-  Communication::InputBuffer response = FetchResponse(name);
-  if (response.IsEmpty())
+  Communication::OutputBuffer request;
+  request << Communication::PROC_GET_PREF << ToUtf8String(name);
+
+  Communication::InputBuffer response;
+  if (!CallEngine(request, response)) 
     return defaultValue;
   bool success;
   response >> success;
   if (success)
   {
     bool value;
     response >> value;
     return value;
   }
   else
     return defaultValue;
 }
 int64_t CAdblockPlusClient::GetPref(const std::wstring& name, int64_t defaultValue)
 {
-  Communication::InputBuffer response = FetchResponse(name);
-  if (response.IsEmpty())
+  Communication::OutputBuffer request;
+  request << Communication::PROC_GET_PREF << ToUtf8String(name);
+
+  Communication::InputBuffer response;
+  if (!CallEngine(request, response)) 
     return defaultValue;
   bool success;
   response >> success;
   if (success)
   {
     int64_t value;
     response >> value;
     return value;
   }
   else
     return defaultValue;
 }