Issues #1163, #1173 - refactor CPluginUserSettings

src/plugin/PluginUserSettings.cpp

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
 #include "PluginStdAfx.h"
 #include "PluginUserSettings.h"
 #include <algorithm>
 #include "PluginSettings.h"
 #include "PluginClient.h"
 #include "../shared/Dictionary.h"
 #include <unordered_map>
 
 namespace
 {
@@ skipping 53 matching lines @@
 
 // ENTRY POINT
 STDMETHODIMP CPluginUserSettings::QueryInterface(REFIID riid, void **ppvObj)
 {
   if (!ppvObj)
   {
     return E_POINTER;
   }
   if (riid == IID_IUnknown || riid == IID_IDispatch) // GUID comparison does not throw
   {
-    *ppvObj = static_cast<void *>(this);
+    *ppvObj = static_cast<void*>(this);
     return S_OK;
   }
   return E_NOINTERFACE;
 }
 
 /**
  * \par Limitation
  *   CPluginUserSettings is not allocated on the heap.
  *   It appears only as a member variable in CPluginTabBase.
  *   'AddRef' and 'Release' don't need reference counting because they don't present COM factories.
@@ skipping 29 matching lines @@
   {
     if (!name || !id)
     {
       return E_POINTER;
     }
     if (count != 1)
     {
       return E_FAIL;
     }
     auto item = methodIndex.find(*name); // unordered_map::find is not declared noexcept
-    if (item==methodIndex.end())
+    if (item == methodIndex.end())
     {
       return DISP_E_UNKNOWNNAME;
     }
     *id = item->second;
   }
   catch (...)
   {
     return E_FAIL;
   }
   return S_OK;
 }
 
 CStringW sGetMessage(const CString& section, const CString& key)
 {
   Dictionary* dictionary = Dictionary::GetInstance();
   return CStringW(dictionary->Lookup(std::string(CW2A(section)), std::string(CW2A(key))).c_str());
 }
 
 STDMETHODIMP CPluginUserSettings::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispparams, VARIANT* pVarResult,
                                          EXCEPINFO* pExcepinfo, UINT* pArgErr)
 {
   try
   {
-    if (!pDispparams || !pExcepinfo)

[sergei, 2015/01/09 15:42:54]

pExcepinfo can be nullptr, it's not an error.

[Eric, 2015/01/09 16:25:24]

I'm mostly trying to add an exception handler in this function, but I don't mind
addressing a few small defects at the same time.

I just checked and we're not even referencing pExcepinfo, much less properly
using it for error conditions.

+    if (!pDispparams)
     {
       return E_POINTER;
     }
     if (pDispparams->cNamedArgs != 0)
     {
       return DISP_E_NONAMEDARGS;
     }
     CPluginSettings* settings = CPluginSettings::GetInstance();
     switch (dispidMember)
     {
     case dispatchID_GetMessage:
       {
         if (pDispparams->cArgs != 2)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_BSTR || pDispparams->rgvarg[1].vt != VT_BSTR)
         {
           return DISP_E_TYPEMISMATCH;
         }
         if (pVarResult)

[sergei, 2015/01/09 15:42:54]

Wouldn't it better here and below to have
if (!pVarResult)
{
  return E_POINTER;
}
CComBSTR key = pDispparams->rgvarg[0].bstrVal;
...
BTW, sometimes there is even no test whether pVarResult is not nullptr.

[Eric, 2015/01/09 16:25:24]

No. It's not an error to ignore the return value. If pVarResult is null, there's
no way to return a value from the invoked method, and thus no point in executing
such a method unless it has side effects.

Apropos, see below for SetLanguage, which is executed solely for its side
effects and does not return a value.

We could use this snippet
    if (!pVarResult)
    {
      return S_OK;
    }
    // ...
if we want to remove a level of indentation for the code below it, but that's a
high degree of change that I'd prefer not do in the present change set.

[sergei, 2015/01/30 16:01:40]

What is about GetAppLocale and below? We should test whether `pVarResult` is not
nullptr.

[Eric, 2015/02/02 14:46:39]

Indeed, there were four defect where 'pVarResult' was not checked against
nullptr before dereferencing. Those are fixed in patch set 5.

For the record, those defects were not introduced in this refactoring, but we
shall take the opportunity to fix them while we're doing this one.

         {
           CComBSTR key = pDispparams->rgvarg[0].bstrVal;
           CComBSTR section = pDispparams->rgvarg[1].bstrVal;
           CStringW message = sGetMessage((BSTR)section, (BSTR)key);
 
           pVarResult->vt = VT_BSTR;
           pVarResult->bstrVal = SysAllocString(message);
         }
       }
       break;
@@ skipping 11 matching lines @@
           pVarResult->lVal = static_cast<LONG>(languageList.size());
         }
       }
       break;
     case dispatchID_GetLanguageByIndex:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        if (VT_I4 != pDispparams->rgvarg[0].vt)
-        {
-          return DISP_E_TYPEMISMATCH;
-        }
-        if (pVarResult)
-        {
-          int indx = pDispparams->rgvarg[0].lVal;
+        if (pDispparams->rgvarg[0].vt != VT_I4)
+        {
+          return DISP_E_TYPEMISMATCH;
+        }
+        if (pVarResult)
+        {
+          int index = pDispparams->rgvarg[0].lVal;
 
           std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
 
-          if (indx < 0  ||  indx >= (int)languageTitleList.size())
+          if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
             return DISP_E_EXCEPTION;
 
           CString language;
 
-          int curIndx = 0;
-          for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
+          int loopIndex = 0;
+          for (std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
-            if (curIndx == indx)
+            if (loopIndex == index)
             {
               language = it->first;
               break;
             }
-            curIndx++;
+            ++loopIndex;
           }
 
           pVarResult->vt = VT_BSTR;
           pVarResult->bstrVal = SysAllocString(language);
         }
       }
       break;
     case dispatchID_GetLanguageTitleByIndex:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        if (VT_I4 != pDispparams->rgvarg[0].vt)
-        {
-          return DISP_E_TYPEMISMATCH;
-        }
-        if (pVarResult)
-        {
-          int indx = pDispparams->rgvarg[0].lVal;
+        if (pDispparams->rgvarg[0].vt != VT_I4)
+        {
+          return DISP_E_TYPEMISMATCH;
+        }
+        if (pVarResult)
+        {
+          int index = pDispparams->rgvarg[0].lVal;
 
           std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
 
-          if (indx < 0  ||  indx >= (int)languageTitleList.size())
+          if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
             return DISP_E_EXCEPTION;
 
           CString languageTitle;
 
-          int curIndx = 0;
-          for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
+          int loopIndex = 0;
+          for (std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
-            if (curIndx == indx)
+            if (loopIndex == index)
             {
               languageTitle = it->second;
               break;
             }
-            curIndx++;
+            loopIndex++;
           }
 
           pVarResult->vt = VT_BSTR;
           pVarResult->bstrVal = SysAllocString(languageTitle);
         }
       }
       break;
     case dispatchID_SetLanguage:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        if (VT_BSTR != pDispparams->rgvarg[0].vt)
+        if (pDispparams->rgvarg[0].vt != VT_BSTR)
         {
           return DISP_E_TYPEMISMATCH;
         }
         CComBSTR url = pDispparams->rgvarg[0].bstrVal;
         settings->SetSubscription((BSTR)url);
       }
       break;
     case dispatchID_GetLanguage:
       {
         if (pDispparams->cArgs != 0)
@@ skipping 30 matching lines @@
           pVarResult->bstrVal = SysAllocString(sWhiteList);
         }
       }
       break;
     case dispatchID_AddWhitelistDomain:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        if (VT_BSTR != pDispparams->rgvarg[0].vt)
+        if (pDispparams->rgvarg[0].vt != VT_BSTR)
         {
           return DISP_E_TYPEMISMATCH;
         }
         CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
         if (domain.Length())
         {
           settings->AddWhiteListedDomain((BSTR)domain);
         }
       }
       break;
     case dispatchID_RemoveWhitelistDomain:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        if (VT_BSTR != pDispparams->rgvarg[0].vt)
+        if (pDispparams->rgvarg[0].vt != VT_BSTR)
         {
           return DISP_E_TYPEMISMATCH;
         }
         CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
         if (domain.Length())
         {
           settings->RemoveWhiteListedDomain((BSTR)domain);
         }
       }
       break;
     case dispatchID_GetAppLocale:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        pVarResult->vt = VT_BSTR;
-        pVarResult->bstrVal = SysAllocString(settings->GetAppLocale());
+        if (pVarResult)
+        {
+          pVarResult->vt = VT_BSTR;
+          pVarResult->bstrVal = SysAllocString(settings->GetAppLocale());
+        }
       }
       break;
     case dispatchID_GetDocumentationLink:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        pVarResult->vt = VT_BSTR;
-        pVarResult->bstrVal = SysAllocString(settings->GetDocumentationLink());
+        if (pVarResult)
+        {
+          pVarResult->vt = VT_BSTR;
+          pVarResult->bstrVal = SysAllocString(settings->GetDocumentationLink());
+        }
       }
       break;
     case dispatchID_IsAcceptableAdsEnabled:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        pVarResult->vt = VT_BOOL;
-        pVarResult->boolVal = CPluginClient::GetInstance()->IsAcceptableAdsEnabled() ? VARIANT_TRUE : VARIANT_FALSE;
+        if (pVarResult)
+        {
+          pVarResult->vt = VT_BOOL;
+          pVarResult->boolVal = CPluginClient::GetInstance()->IsAcceptableAdsEnabled() ? VARIANT_TRUE : VARIANT_FALSE;
+        }
       }
       break;
     case dispatchID_SetAcceptableAdsEnabled:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_BOOL)
         {
@@ skipping 10 matching lines @@
           client->RemoveSubscription(client->GetPref(L"subscriptions_exceptionsurl", L""));
         }
       }
       break;
     case dispatchID_IsUpdate:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
-        pVarResult->vt = VT_BOOL;
-        pVarResult->boolVal = CPluginClient::GetInstance()->GetPref(L"displayUpdatePage", false) ? VARIANT_TRUE : VARIANT_FALSE;
+        if (pVarResult)
+        {
+          pVarResult->vt = VT_BOOL;
+          pVarResult->boolVal = CPluginClient::GetInstance()->GetPref(L"displayUpdatePage", false) ? VARIANT_TRUE : VARIANT_FALSE;
+        }
       }
       break;
     default:
       return DISP_E_MEMBERNOTFOUND;
       break;
     }
   }
-  catch(...)
+  catch (...)
   {
     return E_FAIL;
   }
   return S_OK;
 }
-