Issue #1234 - Eliminate CString from PluginSettings.* and PluginUserSettings.*

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 "AdblockPlusClient.h"
 #include "PluginSettings.h"
+#include "PluginSystem.h"
 #include "../shared/Dictionary.h"
+#include "../shared/Utils.h"
 #include <unordered_map>
 
 namespace
 {
   enum UserSettingsMethods
   {
     dispatchID_GetMessage = 0,
     dispatchID_GetLanguageCount,
     dispatchID_GetLanguageByIndex,
     dispatchID_GetLanguageTitleByIndex,
@@ skipping 110 matching lines @@
     }
     *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());
-}
-

[Eric, 2015/11/20 18:46:04]

Inlined.

 STDMETHODIMP CPluginUserSettings::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispparams, VARIANT* pVarResult,
                                          EXCEPINFO* pExcepinfo, UINT* pArgErr)
 {
   try
   {
     if (!pDispparams)
     {
       return E_POINTER;
     }
     if (pDispparams->cNamedArgs != 0)
@@ skipping 10 matching lines @@
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_BSTR || pDispparams->rgvarg[1].vt != VT_BSTR)
         {
           return DISP_E_TYPEMISMATCH;
         }
         if (pVarResult)
         {
           CComBSTR key = pDispparams->rgvarg[0].bstrVal;
           CComBSTR section = pDispparams->rgvarg[1].bstrVal;
-          CStringW message = sGetMessage((BSTR)section, (BSTR)key);
+          Dictionary* dictionary = Dictionary::GetInstance();
+          std::wstring message = dictionary->Lookup(ToUtf8String(std::wstring(section)), ToUtf8String(std::wstring(key)));

[sergei, 2015/11/25 09:27:08]

Nit: strictly speaking we should use the length obtained via `SysStringLen` and
we don't need ATL::CComBSTR here and below.

[Eric, 2015/11/25 17:31:02]

Done.

There are a few other places where we need this conversion. We probably ought to
define a conversion constructor for it, analogous to the one we have for CString
conversion:

  std::wstring ToWstring( BSTR )

... but not in this change set.
I'm calling this one as an unrelated change that ought to have an issue
association. It's not just in this file; there are plenty other places where
have CComBSTR where we don't need. We don't have a ticket for cleaning up this
usage, and such changes should be linked to one.

[sergei, 2015/11/26 11:06:18]

Sure, feel free to create the issue for it.

 
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(message);
+          pVarResult->bstrVal = SysAllocString(message.c_str());
         }
       }
       break;
     case dispatchID_GetLanguageCount:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pVarResult)
         {
-          std::map<CString, CString> languageList = settings->GetFilterLanguageTitleList();
+          auto languageList = settings->GetFilterLanguageTitleList();
 
           pVarResult->vt = VT_I4;
           pVarResult->lVal = static_cast<LONG>(languageList.size());
         }
       }
       break;
     case dispatchID_GetLanguageByIndex:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         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();
+          auto languageTitleList = settings->GetFilterLanguageTitleList();
 
           if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
-            return DISP_E_EXCEPTION;
+            return DISP_E_BADINDEX;

[Oleksandr, 2015/11/25 03:22:07]

Nit: unrelated change.

[sergei, 2015/11/25 09:27:09]

I would not mind to commit it but it's likely not what we want. IDispatch's
error codes are quite specific for each method and I'm pretty sure that any
caller expects IDispatch::Invoke to return only error codes listed here
https://msdn.microsoft.com/en-us/library/windows/desktop/ms221479(v=vs.85).aspx.
So, only DISP_E_EXCEPTION seems to be the correct return value for custom
errors.

[Eric, 2015/11/25 17:31:02]

It fixes a defect, actually. Rather than bicker over it here, I've reverted it
and created a new ticket describing the defect.

https://issues.adblockplus.org/ticket/3350
Microsoft Excel returns DISP_E_BADINDEX, as a web search clearly shows. See, for
example,
http://stackoverflow.com/questions/16851642/c-sharp-excel-interop-exception-f...
where the .NET runtime correctly converts the numeric HRESULT to both its
symbolic origin and also a human language error message (albeit an unclear one).
The point here is the Microsoft is supporting this return value, even if they
haven't bothered to update their documentation.
That same page you referenced says the following about DISP_E_EXCEPTION:

    The application needs to raise an exception. In this case, the structure
    passed in pexcepinfo should be filled in. 

The defect is that we're not filling in any exception structure.

 
-          CString language;
+          std::wstring language;
 
           int loopIndex = 0;
-          for (std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
+          for (auto it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
             if (loopIndex == index)
             {
               language = it->first;
               break;
             }
             ++loopIndex;
           }
 
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(language);
+          pVarResult->bstrVal = SysAllocString(language.c_str());
         }
       }
       break;
     case dispatchID_GetLanguageTitleByIndex:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         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();
+          auto languageTitleList = settings->GetFilterLanguageTitleList();
 
           if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
-            return DISP_E_EXCEPTION;
+            return DISP_E_BADINDEX;

[Oleksandr, 2015/11/25 03:22:08]

Nit: unrelated change.

 
-          CString languageTitle;
-
+          std::wstring languageTitle;
           int loopIndex = 0;
-          for (std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
+          for (auto it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
             if (loopIndex == index)
             {
               languageTitle = it->second;
               break;
             }
             loopIndex++;
           }
 
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(languageTitle);
+          pVarResult->bstrVal = SysAllocString(languageTitle.c_str());
         }
       }
       break;
     case dispatchID_SetLanguage:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         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)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pVarResult)
         {
-          CString url = settings->GetSubscription();
+          std::wstring url = settings->GetSubscription();
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(url);
+          pVarResult->bstrVal = SysAllocString(url.c_str());
         }
       }
       break;
     case dispatchID_GetWhitelistDomains:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pVarResult)
         {
-          std::vector<std::wstring> whiteList = settings->GetWhiteListedDomainList();
-          CString sWhiteList;
-          for (size_t i = 0; i < whiteList.size(); i++)
+          auto whiteListDomains = settings->GetWhiteListedDomainList();
+          std::wstring commaSeparatedDomains;
+          for (size_t i = 0; i < whiteListDomains.size(); i++)
           {
-            if (!sWhiteList.IsEmpty())
+            if (!commaSeparatedDomains.empty())
             {
-              sWhiteList += ',';
+              commaSeparatedDomains += ',';
             }
-            sWhiteList += CString(whiteList[i].c_str());
+            commaSeparatedDomains += whiteListDomains[i];
           }
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(sWhiteList);
+          pVarResult->bstrVal = SysAllocString(commaSeparatedDomains.c_str());
         }
       }
       break;
     case dispatchID_AddWhitelistDomain:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_BSTR)
@@ skipping 13 matching lines @@
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_BSTR)
         {
           return DISP_E_TYPEMISMATCH;
         }
         CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
         if (domain.Length())
         {
-          settings->RemoveWhiteListedDomain((BSTR)domain);
+          settings->RemoveWhiteListedDomain(std::wstring(domain));
         }
       }
       break;
     case dispatchID_GetAppLocale:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pVarResult)
         {
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(settings->GetAppLocale());
+          pVarResult->bstrVal = SysAllocString(GetBrowserLanguage().c_str());
         }
       }
       break;
     case dispatchID_GetDocumentationLink:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pVarResult)
         {
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(settings->GetDocumentationLink());
+          pVarResult->bstrVal = SysAllocString(CPluginClient::GetInstance()->GetDocumentationLink().c_str());
         }
       }
       break;
     case dispatchID_IsAcceptableAdsEnabled:
       {
         if (pDispparams->cArgs != 0)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pVarResult)
@@ skipping 42 matching lines @@
       return DISP_E_MEMBERNOTFOUND;
       break;
     }
   }
   catch (...)
   {
     return E_FAIL;
   }
   return S_OK;
 }