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

src/plugin/PluginUserSettings.cpp

Index: src/plugin/PluginUserSettings.cpp
===================================================================
--- a/src/plugin/PluginUserSettings.cpp
+++ b/src/plugin/PluginUserSettings.cpp
@@ -19,7 +19,9 @@
 #include "PluginUserSettings.h"
 #include "AdblockPlusClient.h"
 #include "PluginSettings.h"
+#include "PluginSystem.h"
 #include "../shared/Dictionary.h"
+#include "../shared/Utils.h"
 #include <unordered_map>
 
 namespace
@@ -150,12 +152,6 @@
   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)
 {
@@ -186,10 +182,11 @@
         {
           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;
@@ -201,7 +198,7 @@
         }
         if (pVarResult)
         {
-          std::map<CString, CString> languageList = settings->GetFilterLanguageTitleList();
+          auto languageList = settings->GetFilterLanguageTitleList();
 
           pVarResult->vt = VT_I4;
           pVarResult->lVal = static_cast<LONG>(languageList.size());
@@ -222,15 +219,15 @@
         {
           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)
             {
@@ -241,7 +238,7 @@
           }
 
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(language);
+          pVarResult->bstrVal = SysAllocString(language.c_str());
         }
       }
       break;
@@ -259,15 +256,14 @@
         {
           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)
             {
@@ -278,7 +274,7 @@
           }
 
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(languageTitle);
+          pVarResult->bstrVal = SysAllocString(languageTitle.c_str());
         }
       }
       break;
@@ -304,9 +300,9 @@
         }
         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;
@@ -318,18 +314,18 @@
         }
         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;
@@ -363,7 +359,7 @@
         CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
         if (domain.Length())
         {
-          settings->RemoveWhiteListedDomain((BSTR)domain);
+          settings->RemoveWhiteListedDomain(std::wstring(domain));
         }
       }
       break;
@@ -376,7 +372,7 @@
         if (pVarResult)
         {
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(settings->GetAppLocale());
+          pVarResult->bstrVal = SysAllocString(GetBrowserLanguage().c_str());
         }
       }
       break;
@@ -389,7 +385,7 @@
         if (pVarResult)
         {
           pVarResult->vt = VT_BSTR;
-          pVarResult->bstrVal = SysAllocString(settings->GetDocumentationLink());
+          pVarResult->bstrVal = SysAllocString(CPluginClient::GetInstance()->GetDocumentationLink().c_str());
         }
       }
       break;