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
@@ skipping 165 matching lines @@
         }
         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;
           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.

+          std::wstring message = dictionary->Lookup(
+            ToUtf8String(std::wstring(section, ::SysStringLen(section))),
+            ToUtf8String(std::wstring(key, ::SysStringLen(key)))
+          );
 
           pVarResult->vt = VT_BSTR;
           pVarResult->bstrVal = SysAllocString(message.c_str());
         }
       }
       break;
     case dispatchID_GetLanguageCount:
       {
         if (pDispparams->cArgs != 0)
         {
@@ skipping 18 matching lines @@
         {
           return DISP_E_TYPEMISMATCH;
         }
         if (pVarResult)
         {
           int index = pDispparams->rgvarg[0].lVal;
 
           auto languageTitleList = settings->GetFilterLanguageTitleList();
 
           if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
-            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.

+            return DISP_E_EXCEPTION;
 
           std::wstring language;
 
           int loopIndex = 0;
           for (auto it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
             if (loopIndex == index)
             {
               language = it->first;
               break;
@@ skipping 16 matching lines @@
         {
           return DISP_E_TYPEMISMATCH;
         }
         if (pVarResult)
         {
           int index = pDispparams->rgvarg[0].lVal;
 
           auto languageTitleList = settings->GetFilterLanguageTitleList();
 
           if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
-            return DISP_E_BADINDEX;

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

Nit: unrelated change.

+            return DISP_E_EXCEPTION;
 
           std::wstring languageTitle;
           int loopIndex = 0;
           for (auto it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
             if (loopIndex == index)
             {
               languageTitle = it->second;
               break;
             }
@@ skipping 168 matching lines @@
       return DISP_E_MEMBERNOTFOUND;
       break;
     }
   }
   catch (...)
   {
     return E_FAIL;
   }
   return S_OK;
 }