Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: src/plugin/PluginUserSettings.cpp

Issue 5979857238360064: Issues #1163, #1173 - refactor CPluginUserSettings (Closed)
Patch Set: Created Aug. 6, 2014, 4:56 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: src/plugin/PluginUserSettings.cpp
===================================================================
--- a/src/plugin/PluginUserSettings.cpp
+++ b/src/plugin/PluginUserSettings.cpp
@@ -4,358 +4,399 @@
#include "PluginSettings.h"
#include "PluginClient.h"
#include "../shared/Dictionary.h"
+#include <unordered_map>
-static const CString s_GetMessage = L"GetMessage";
-static const CString s_GetLanguageCount = L"GetLanguageCount";
-static const CString s_GetLanguageByIndex = L"GetLanguageByIndex";
-static const CString s_GetLanguageTitleByIndex = L"GetLanguageTitleByIndex";
-static const CString s_SetLanguage = L"SetLanguage";
-static const CString s_GetLanguage = L"GetLanguage";
-static const CString s_GetWhitelistDomains = L"GetWhitelistDomains";
-static const CString s_AddWhitelistDomain = L"AddWhitelistDomain";
-static const CString s_RemoveWhitelistDomain = L"RemoveWhitelistDomain";
-static const CString s_GetAppLocale = L"GetAppLocale";
-static const CString s_GetDocumentationLink = L"GetDocumentationLink";
-static const CString s_IsAcceptableAdsEnabled = L"IsAcceptableAdsEnabled";
-static const CString s_SetAcceptableAdsEnabled = L"SetAcceptableAdsEnabled";
-static const CString s_Methods[] = {s_GetMessage, s_GetLanguageCount, s_GetLanguageByIndex, s_GetLanguageTitleByIndex, s_SetLanguage, s_GetLanguage, s_GetWhitelistDomains, s_AddWhitelistDomain, s_RemoveWhitelistDomain, s_GetAppLocale, s_GetDocumentationLink, s_IsAcceptableAdsEnabled, s_SetAcceptableAdsEnabled};
+namespace
+{
+ enum UserSettingsMethods
+ {
+ dispatchID_GetMessage = 0,
+ dispatchID_GetLanguageCount,
+ dispatchID_GetLanguageByIndex,
+ dispatchID_GetLanguageTitleByIndex,
+ dispatchID_SetLanguage,
+ dispatchID_GetLanguage,
+ dispatchID_GetWhitelistDomains,
+ dispatchID_AddWhitelistDomain,
+ dispatchID_RemoveWhitelistDomain,
+ dispatchID_GetAppLocale,
+ dispatchID_GetDocumentationLink,
+ dispatchID_IsAcceptableAdsEnabled,
+ dispatchID_SetAcceptableAdsEnabled
+ };
-CPluginUserSettings::CPluginUserSettings()
-{
+ std::unordered_map<std::wstring, DISPID> InitMethodIndex()
+ {
+ std::unordered_map<std::wstring, DISPID> m;
+ // try-block for safety during static initialization
+ try
+ {
+ m.emplace(L"GetMessage", dispatchID_GetMessage);
Oleksandr 2014/08/17 22:52:01 It's outside the scope of this review, but since w
Eric 2014/09/29 18:45:41 Discussion points. 1. I wouldn't mind breaking up
Oleksandr 2014/10/02 20:36:53 Creating a low priority issue in our issue tracker
Eric 2014/10/14 22:23:50 I wrote an issue for the slightly larger task of c
+ m.emplace(L"GetLanguageCount", dispatchID_GetLanguageCount);
+ m.emplace(L"GetLanguageByIndex", dispatchID_GetLanguageByIndex);
+ m.emplace(L"GetLanguageTitleByIndex", dispatchID_GetLanguageTitleByIndex);
+ m.emplace(L"SetLanguage", dispatchID_SetLanguage);
+ m.emplace(L"GetLanguage", dispatchID_GetLanguage);
+ m.emplace(L"GetWhitelistDomains", dispatchID_GetWhitelistDomains);
+ m.emplace(L"AddWhitelistDomain", dispatchID_AddWhitelistDomain);
+ m.emplace(L"RemoveWhitelistDomain", dispatchID_RemoveWhitelistDomain);
+ m.emplace(L"GetAppLocale", dispatchID_GetAppLocale);
+ m.emplace(L"GetDocumentationLink", dispatchID_GetDocumentationLink);
+ m.emplace(L"IsAcceptableAdsEnabled", dispatchID_IsAcceptableAdsEnabled);
+ m.emplace(L"SetAcceptableAdsEnabled", dispatchID_SetAcceptableAdsEnabled);
+ }
+ catch(...)
+ {
+ }
+ return m;
+ }
+
+ std::unordered_map<std::wstring, DISPID> methodIndex = InitMethodIndex();
}
-
STDMETHODIMP CPluginUserSettings::QueryInterface(REFIID riid, void **ppvObj)
{
- if (IID_IUnknown == riid || IID_IDispatch == riid)
+ // Entry point, but no exception handler; nothing here can throw
Oleksandr 2014/08/17 22:52:01 This comment and all the others concerning the exc
Eric 2014/09/29 18:45:41 I would like to have some kind of documentation in
Oleksandr 2014/10/02 20:36:53 In that case, it would seem less intrusive to have
+ if (!ppvObj)
{
- *ppvObj = (LPVOID)this;
- return NOERROR;
+ return E_POINTER;
}
-
+ if (riid == IID_IUnknown || riid == IID_IDispatch) // GUID comparison does not throw
+ {
+ *ppvObj = static_cast<void *>(this);
+ return S_OK;
+ }
return E_NOINTERFACE;
}
-
-/*
-Since CPluginUserSettings is not allocated on the heap, 'AddRef' and 'Release' don't need reference counting,
-because CPluginUserSettings won't be deleted when reference counter == 0
-*/
-
+/**
+ * \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.
+ */
ULONG __stdcall CPluginUserSettings::AddRef()
{
return 1;
}
-
ULONG __stdcall CPluginUserSettings::Release()
{
return 1;
}
-
STDMETHODIMP CPluginUserSettings::GetTypeInfoCount(UINT* pctinfo)
{
return E_NOTIMPL;
}
-
STDMETHODIMP CPluginUserSettings::GetTypeInfo(UINT itinfo, LCID lcid, ITypeInfo** pptinfo)
{
return E_NOTIMPL;
}
-
-STDMETHODIMP CPluginUserSettings::GetIDsOfNames(REFIID riid, LPOLESTR* rgszNames, UINT cNames, LCID lcid, DISPID* rgdispid)
+/**
+ * \par Limitation
+ * The specification for this method in IDispatch maps an array of names to an array of identifiers.
+ * This version only supports single-element arrays, which is enough for IE's JavaScript interpreter.
+ */
+STDMETHODIMP CPluginUserSettings::GetIDsOfNames(REFIID, LPOLESTR* name, UINT count, LCID, DISPID* id)
{
- if (!rgszNames)
- return E_POINTER;
-
- if (!rgdispid)
- return E_POINTER;
-
- if (1 != cNames)
+ // Entry point exception handler
+ try
+ {
+ 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())
+ {
+ return DISP_E_UNKNOWNNAME;
+ }
+ *id = item->second;
+ }
+ catch(...)
+ {
return E_FAIL;
-
- size_t indxMethod = 0;
- for (; indxMethod < countof(s_Methods); indxMethod++)
- {
- if (*rgszNames == s_Methods[indxMethod])
- break;
}
-
- if (indxMethod == countof(s_Methods))
- return DISP_E_MEMBERNOTFOUND;
-
- *rgdispid = static_cast<DISPID>(indxMethod);
-
return S_OK;
}
-
-static CString sGetLanguage()
-{
- CPluginSettings* settings = CPluginSettings::GetInstance();
- return settings->GetSubscription();
-}
-
-
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());
}
-std::wstring sGetMessage(const std::string& section, const std::string& key)
-{
- Dictionary* dictionary = Dictionary::GetInstance();
- return dictionary->Lookup(section, key);
-}
-
-
STDMETHODIMP CPluginUserSettings::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispparams, VARIANT* pVarResult,
EXCEPINFO* pExcepinfo, UINT* pArgErr)
{
- if (!pDispparams)
- return E_POINTER;
+ // Entry point exception handler
+ try
+ {
+ if (!pDispparams || !pExcepinfo)
+ {
+ return E_POINTER;
+ }
+ if (pDispparams->cNamedArgs != 0)
+ {
+ return DISP_E_NONAMEDARGS;
+ }
- if (!pExcepinfo)
- return E_POINTER;
+ CPluginSettings* settings = CPluginSettings::GetInstance();
- if (pDispparams->cNamedArgs)
- return DISP_E_NONAMEDARGS;
+ switch (dispidMember)
+ {
+ case dispatchID_GetMessage:
+ {
+ if (2 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
- CPluginSettings* settings = CPluginSettings::GetInstance();
+ if (VT_BSTR != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
+ if (VT_BSTR != pDispparams->rgvarg[1].vt)
+ return DISP_E_TYPEMISMATCH;
- if (dispidMember < 0 || dispidMember >= countof(s_Methods))
- return DISP_E_BADINDEX;
+ if (pVarResult)
+ {
+ CComBSTR key = pDispparams->rgvarg[0].bstrVal;
+ CComBSTR section = pDispparams->rgvarg[1].bstrVal;
+ CStringW message = sGetMessage((BSTR)section, (BSTR)key);
- const CString& method = s_Methods[dispidMember];
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(message);
+ }
+ }
+ break;
+ case dispatchID_GetLanguageCount:
+ {
+ if (pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
- if (s_GetMessage == method)
- {
- if (2 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
+ if (pVarResult)
+ {
+ std::map<CString, CString> languageList = settings->GetFilterLanguageTitleList();
- if (VT_BSTR != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
+ pVarResult->vt = VT_I4;
+ pVarResult->lVal = static_cast<LONG>(languageList.size());
+ }
+ }
+ break;
+ case dispatchID_GetLanguageByIndex:
+ {
+ if (1 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
- if (pVarResult)
- {
- CComBSTR key = pDispparams->rgvarg[0].bstrVal;
- CComBSTR section = pDispparams->rgvarg[1].bstrVal;
- CStringW message = sGetMessage((BSTR)section, (BSTR)key);
+ if (VT_I4 != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(message);
+ if (pVarResult)
+ {
+ int indx = pDispparams->rgvarg[0].lVal;
+
+ std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
+
+ if (indx < 0 || indx >= (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)
+ {
+ if (curIndx == indx)
+ {
+ language = it->first;
+ break;
+ }
+
+ curIndx++;
+ }
+
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(language);
+ }
+ }
+ break;
+ case dispatchID_GetLanguageTitleByIndex:
+ {
+ if (1 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (VT_I4 != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
+
+ if (pVarResult)
+ {
+ int indx = pDispparams->rgvarg[0].lVal;
+
+ std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
+
+ if (indx < 0 || indx >= (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)
+ {
+ if (curIndx == indx)
+ {
+ languageTitle = it->second;
+ break;
+ }
+
+ curIndx++;
+ }
+
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(languageTitle);
+ }
+ }
+ break;
+ case dispatchID_SetLanguage:
+ {
+ if (1 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (VT_BSTR != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
+
+ CComBSTR url = pDispparams->rgvarg[0].bstrVal;
+
+ settings->SetSubscription((BSTR)url);
+ }
+ break;
+ case dispatchID_GetLanguage:
+ {
+ if (pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (pVarResult)
+ {
+ CString url = settings->GetSubscription();
+
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(url);
+ }
+ }
+ break;
+ case dispatchID_GetWhitelistDomains:
+ {
+ if (pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (pVarResult)
+ {
+ std::vector<std::wstring> whiteList = settings->GetWhiteListedDomainList();
+ CString sWhiteList;
+ for (size_t i = 0; i < whiteList.size(); i++)
+ {
+ if (!sWhiteList.IsEmpty())
+ {
+ sWhiteList += ',';
+ }
+ sWhiteList += CString(whiteList[i].c_str());
+ }
+
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(sWhiteList);
+ }
+ }
+ break;
+ case dispatchID_AddWhitelistDomain:
+ {
+ if (1 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (VT_BSTR != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
+
+ CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
+ if (domain.Length())
+ {
+ settings->AddWhiteListedDomain((BSTR)domain);
+ }
+ }
+ break;
+ case dispatchID_RemoveWhitelistDomain:
+ {
+ if (1 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (VT_BSTR != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
+
+ CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
+ if (domain.Length())
+ {
+ settings->RemoveWhiteListedDomain((BSTR)domain);
+ }
+ }
+ break;
+ case dispatchID_GetAppLocale:
+ {
+ if (0 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(settings->GetAppLocale());
+ }
+ break;
+ case dispatchID_GetDocumentationLink:
+ {
+ if (0 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ pVarResult->vt = VT_BSTR;
+ pVarResult->bstrVal = SysAllocString(settings->GetDocumentationLink());
+ }
+ break;
+ case dispatchID_IsAcceptableAdsEnabled:
+ {
+ if (0 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ pVarResult->vt = VT_BOOL;
+ pVarResult->boolVal = CPluginClient::GetInstance()->IsAcceptableAdsEnabled();
+ }
+ break;
+ case dispatchID_SetAcceptableAdsEnabled:
+ {
+ if (1 != pDispparams->cArgs)
+ return DISP_E_BADPARAMCOUNT;
+
+ if (VT_BOOL != pDispparams->rgvarg[0].vt)
+ return DISP_E_TYPEMISMATCH;
+
+ bool enable = pDispparams->rgvarg[0].boolVal;
+
+ if (enable)
+ {
+ CPluginClient* client = CPluginClient::GetInstance();
+ client->AddSubscription(client->GetPref(L"subscriptions_exceptionsurl", L""));
+ }
+ else
+ {
+ CPluginClient* client = CPluginClient::GetInstance();
+ client->RemoveSubscription(client->GetPref(L"subscriptions_exceptionsurl", L""));
+ }
+ }
+ break;
+ default:
+ return DISP_E_MEMBERNOTFOUND;
+ break;
}
}
- else if (s_GetLanguageCount == method)
+ catch(...)
{
- if (pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (pVarResult)
- {
- std::map<CString, CString> languageList = settings->GetFilterLanguageTitleList();
-
- pVarResult->vt = VT_I4;
- pVarResult->lVal = static_cast<LONG>(languageList.size());
- }
+ return E_FAIL;
}
- else if (s_GetLanguageByIndex == method)
- {
- if (1 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (VT_I4 != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
-
- if (pVarResult)
- {
- int indx = pDispparams->rgvarg[0].lVal;
-
- std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
-
- if (indx < 0 || indx >= (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)
- {
- if (curIndx == indx)
- {
- language = it->first;
- break;
- }
-
- curIndx++;
- }
-
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(language);
- }
- }
- else if (s_GetLanguageTitleByIndex == method)
- {
- if (1 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (VT_I4 != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
-
- if (pVarResult)
- {
- int indx = pDispparams->rgvarg[0].lVal;
-
- std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
-
- if (indx < 0 || indx >= (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)
- {
- if (curIndx == indx)
- {
- languageTitle = it->second;
- break;
- }
-
- curIndx++;
- }
-
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(languageTitle);
- }
- }
- else if (s_SetLanguage == method)
- {
- if (1 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (VT_BSTR != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
-
- CComBSTR url = pDispparams->rgvarg[0].bstrVal;
-
- settings->SetSubscription((BSTR)url);
- }
- else if (s_GetLanguage == method)
- {
- if (pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (pVarResult)
- {
- CString url = settings->GetSubscription();
-
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(url);
- }
- }
- else if (s_GetWhitelistDomains == method)
- {
- if (pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (pVarResult)
- {
- std::vector<std::wstring> whiteList = settings->GetWhiteListedDomainList();
- CString sWhiteList;
- for (size_t i = 0; i < whiteList.size(); i++)
- {
- if (!sWhiteList.IsEmpty())
- {
- sWhiteList += ',';
- }
- sWhiteList += CString(whiteList[i].c_str());
- }
-
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(sWhiteList);
- }
- }
- else if (s_AddWhitelistDomain == method)
- {
- if (1 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (VT_BSTR != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
-
- CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
- if (domain.Length())
- {
- settings->AddWhiteListedDomain((BSTR)domain);
- }
- }
- else if (s_RemoveWhitelistDomain == method)
- {
- if (1 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (VT_BSTR != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
-
- CComBSTR domain = pDispparams->rgvarg[0].bstrVal;
- if (domain.Length())
- {
- settings->RemoveWhiteListedDomain((BSTR)domain);
- }
- }
- else if (s_GetAppLocale == method)
- {
- if (0 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(settings->GetAppLocale());
- }
- else if (s_GetDocumentationLink == method)
- {
- if (0 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- pVarResult->vt = VT_BSTR;
- pVarResult->bstrVal = SysAllocString(settings->GetDocumentationLink());
- }
- else if (s_IsAcceptableAdsEnabled == method)
- {
- if (0 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- pVarResult->vt = VT_BOOL;
- pVarResult->boolVal = CPluginClient::GetInstance()->IsAcceptableAdsEnabled();
- }
- else if (s_SetAcceptableAdsEnabled == method)
- {
- if (1 != pDispparams->cArgs)
- return DISP_E_BADPARAMCOUNT;
-
- if (VT_BOOL != pDispparams->rgvarg[0].vt)
- return DISP_E_TYPEMISMATCH;
-
- bool enable = pDispparams->rgvarg[0].boolVal;
-
- if (enable)
- {
- CPluginClient* client = CPluginClient::GetInstance();
- client->AddSubscription(client->GetPref(L"subscriptions_exceptionsurl", L""));
- }
- else
- {
- CPluginClient* client = CPluginClient::GetInstance();
- client->RemoveSubscription(client->GetPref(L"subscriptions_exceptionsurl", L""));
- }
- }
- else
- return DISP_E_MEMBERNOTFOUND;
-
return S_OK;
}

Powered by Google App Engine
This is Rietveld