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

Unified Diff: src/plugin/PluginClass.cpp

Issue 4912420225024000: Issue #1234 - Convert strings associated with URL's (Closed)
Patch Set: Created Oct. 14, 2014, 10:17 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/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -202,26 +202,24 @@
return browser;
}
-CString CPluginClass::GetBrowserUrl() const
+std::wstring CPluginClass::GetBrowserUrl() const
{
- CString url;
-
+ std::wstring url;
CComQIPtr<IWebBrowser2> browser = GetBrowser();
if (browser)
{
- CComBSTR bstrURL;
-
- if (SUCCEEDED(browser->get_LocationURL(&bstrURL)))
+ BSTR bstrURL;
sergei 2014/10/17 10:10:12 Why is ATL::CComBSTR replaced by BSTR? Looks like
Eric 2014/10/20 02:36:01 Because we don't need CComBSTR here. The only reas
sergei 2014/10/21 09:45:17 Sorry, I don't understand about which mystery your
Eric 2014/10/21 17:19:52 That's because there is no mystery. I take it, th
Oleksandr 2015/01/05 11:55:40 Let's move back to this specific code change. As I
Eric 2015/01/05 16:18:26 Done.
+ if (SUCCEEDED(browser->get_LocationURL(&bstrURL)) && bstrURL)
{
- url = bstrURL;
- CPluginClient::UnescapeUrl(url);
+ url = std::wstring(bstrURL, SysStringLen(bstrURL));
+ SysFreeString(bstrURL);
+ UnescapeUrl(url);
}
}
else
{
url = m_tab->GetDocumentUrl();
}
-
return url;
}
@@ -549,13 +547,17 @@
}
// Get the URL
- CString url;
- vt = pDispParams->rgvarg[5].vt;
- if (vt == VT_BYREF + VT_VARIANT)
+ std::wstring url;
+ auto arg = pDispParams->rgvarg[5];
sergei 2014/10/17 10:10:12 I would prefer `const auto&` here.
Eric 2014/10/20 02:36:01 OK.
+ vt = arg.vt;
+ if (vt == VT_BYREF + VT_VARIANT && arg.pvarVal->vt == VT_BSTR)
sergei 2014/10/17 10:10:12 '+' here looks unusual, despite it works, I think
Eric 2014/10/20 02:36:01 It's out of scope, but it's also trivial. Done.
{
- url = pDispParams->rgvarg[5].pvarVal->bstrVal;
-
- CPluginClient::UnescapeUrl(url);
+ BSTR b = arg.pvarVal->bstrVal;
+ if (b) {
+ url = std::wstring(b, SysStringLen(b));
+ SysFreeString(b);
sergei 2014/10/17 10:10:12 It's not allowed to modify arguments, especially b
Eric 2014/10/20 02:36:01 Brain fart. Forgot that this was deriving from IDi
+ UnescapeUrl(url);
+ }
}
else
{
@@ -565,28 +567,29 @@
// If webbrowser2 is equal to top level browser (as set in SetSite), we are navigating new page
CPluginClient* client = CPluginClient::GetInstance();
-
- if (url.Find(L"javascript") == 0)
+ CString urlLegacy = ToCString(url);
+ if (urlLegacy.Find(L"javascript") == 0)
{
}
else if (GetBrowser().IsEqualObject(WebBrowser2Ptr))
{
m_tab->OnNavigate(url);
- DEBUG_GENERAL(L"================================================================================\nBegin main navigation url:" + url + "\n================================================================================")
+ DEBUG_GENERAL(L"================================================================================\nBegin main navigation url:" + urlLegacy + "\n================================================================================")
#ifdef ENABLE_DEBUG_RESULT
- CPluginDebug::DebugResultDomain(url);
+ CPluginDebug::DebugResultDomain(urlLegacy);
#endif
UpdateStatusBar();
}
else
{
- DEBUG_NAVI(L"Navi::Begin navigation url:" + url)
+ DEBUG_NAVI(L"Navi::Begin navigation url:" + urlLegacy)
m_tab->CacheFrame(url);
}
}
+
STDMETHODIMP CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
{
DEBUG_GENERAL("Tab changed");
@@ -719,14 +722,12 @@
CComQIPtr<IWebBrowser2> pBrowser = pDispParams->rgvarg[1].pdispVal;
if (pBrowser)
{
- CString url;
- CComBSTR bstrUrl;
+ BSTR bstrUrl;
sergei 2014/10/17 10:10:12 Again CComBSTR is replaced by BSTR.
Eric 2014/10/20 02:36:01 And for the same reasons I stated above.
if (SUCCEEDED(pBrowser->get_LocationURL(&bstrUrl)) && ::SysStringLen(bstrUrl) > 0)
{
- url = bstrUrl;
-
- CPluginClient::UnescapeUrl(url);
-
+ std::wstring url(bstrUrl, SysStringLen(bstrUrl));
+ SysFreeString(bstrUrl);
+ UnescapeUrl(url);
m_tab->OnDocumentComplete(browser, url, browser.IsEqualObject(pBrowser));
}
}
@@ -1140,7 +1141,7 @@
return S_OK;
}
-HMENU CPluginClass::CreatePluginMenu(const CString& url)
+HMENU CPluginClass::CreatePluginMenu(const std::wstring& url)
{
DEBUG_GENERAL("CreatePluginMenu");
HINSTANCE hInstance = _AtlBaseModule.GetModuleInstance();
@@ -1357,14 +1358,14 @@
case ID_MENU_DISABLE_ON_SITE:
{
CPluginSettings* settings = CPluginSettings::GetInstance();
- CString urlString = GetTab()->GetDocumentUrl();
- if (client->IsWhitelistedUrl(to_wstring(urlString)))
+ std::wstring urlString = GetTab()->GetDocumentUrl();
+ if (client->IsWhitelistedUrl(urlString))
{
- settings->RemoveWhiteListedDomain(to_CString(client->GetHostFromUrl(to_wstring(urlString))));
+ settings->RemoveWhiteListedDomain(to_CString(client->GetHostFromUrl(urlString)));
}
else
{
- settings->AddWhiteListedDomain(to_CString(client->GetHostFromUrl(to_wstring(urlString))));
+ settings->AddWhiteListedDomain(to_CString(client->GetHostFromUrl(urlString)));
}
GetBrowser()->Refresh();
}
@@ -1377,7 +1378,7 @@
}
-bool CPluginClass::SetMenuBar(HMENU hMenu, const CString& url)
+bool CPluginClass::SetMenuBar(HMENU hMenu, const std::wstring& url)
{
DEBUG_GENERAL("SetMenuBar");
@@ -1406,8 +1407,8 @@
{
ctext = dictionary->Lookup("menu", "menu-disable-on-site");
// Is domain in white list?
- ReplaceString(ctext, L"?1?", client->GetHostFromUrl(to_wstring(url)));
- if (client->IsWhitelistedUrl(to_wstring(GetTab()->GetDocumentUrl())))
+ ReplaceString(ctext, L"?1?", client->GetHostFromUrl(url));
+ if (client->IsWhitelistedUrl(GetTab()->GetDocumentUrl()))
{
fmii.fState = MFS_CHECKED | MFS_ENABLED;
}
@@ -1622,7 +1623,7 @@
}
-HICON CPluginClass::GetStatusBarIcon(const CString& url)
+HICON CPluginClass::GetStatusBarIcon(const std::wstring& url)
{
// use the disable icon as defualt, if the client doesn't exists
HICON hIcon = GetIcon(ICON_PLUGIN_DEACTIVATED);
@@ -1634,7 +1635,7 @@
if (!CPluginSettings::GetInstance()->IsPluginEnabled())
{
}
- else if (client->IsWhitelistedUrl(to_wstring(url)))
+ else if (client->IsWhitelistedUrl(url))
{
hIcon = GetIcon(ICON_PLUGIN_DISABLED);
}
@@ -1751,14 +1752,14 @@
case WM_LBUTTONUP:
case WM_RBUTTONUP:
{
- CString strURL = pClass->GetBrowserUrl();
- if (strURL != pClass->GetTab()->GetDocumentUrl())
+ std::wstring url = pClass->GetBrowserUrl();
+ if (url != pClass->GetTab()->GetDocumentUrl())
{
- pClass->GetTab()->SetDocumentUrl(strURL);
+ pClass->GetTab()->SetDocumentUrl(url);
}
// Create menu
- HMENU hMenu = pClass->CreatePluginMenu(strURL);
+ HMENU hMenu = pClass->CreatePluginMenu(url);
if (!hMenu)
{
return 0;

Powered by Google App Engine
This is Rietveld