Issue #1234 - Convert strings associated with URL's

src/plugin/PluginClass.cpp

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 degradation.

[Eric, 2014/10/20 02:36:01]

Because we don't need CComBSTR here. The only reason to use it is to call
SysFreeString. Eliminating it improves the code by showing explicitly what the
BSTR needs. There's no mysterious secret hiding within CComBSTR.

CComBSTR doesn't convert naturally to std::wstring like our own class would,
either. Having such a conversion improves the readability of the code
significantly (I know because I've already done it once). Keeping the shroud of
mystery around CComBSTR just keeps the code from moving forward.

[sergei, 2014/10/21 09:45:17]

Sorry, I don't understand about which mystery your talking.
The main rule is to use RAII, ATL::CComBSTR completely satisfies it and it's
completely enough for us in my personal opinion. Using manual resource
management is a step backward.
What concerns using our own wrapper around BSTR. First of all we should ask
whether we need it or not, I am not convinced that we need it. Secondly, to
replace ATL::CComBSTR by it seems easier than to replace
BSTR+SysStringLen+SysFreeString

[Eric, 2014/10/21 17:19:52]

That's because there is no mystery.

I take it, then, that you do understand that all we need here is to call
SysFreeString in a destructor, whether that be in CComBSTR or in some other
wrapper class.
I agree about RAII, but CComBSTR is not the only way of accomplishing it.

Continuing to use CComBSTR is failing to step forward. We did not decide to
enshrine ATL; we decided to de-prioritize removing it.
Somehow, even though I keep repeating them, I haven't seen you acknowledge that
there are advantages to using our own wrapper class. We haven't even got to the
point where we are able to weigh advantages vs. disadvantages. 

So, once more, there are several advantages to having our own wrapper class for
this case, namely, [in] arguments on the client side:

1) Built-in conversion operator to std::wstring().
  a) Without such an operator, each and every conversion requires
    i) A condition to check that the BSTR value (raw or wrapped) is not null,
and
    ii) An explicit call to SysStringLen(), because BSTR are not guaranteed to
be
        null-terminated.

2) We can make all the tests for empty strings consistent by calling 'empty()'.
  a) The code at present does not
  
3) Its constructor can be declared 'noexcept', since it doesn't allocate.
  a) Don't expect Microsoft to update ATL for C++11.

4) Throw an exception for erroneous uses of 'operator&'.
  a) The ATL version does not do so in Release configurations
  b) We can throw 'std::logic_error' instead of the non-'std' ATL exception.
If we were to use CComBSTR, we cannot replace SysStringLen. BSTR is not
guaranteed to be null-terminated, so we cannot use the simpler constructor for
'wstring'. That means that if we enshrine CComBSTR, we enshrine explicit calls
to SysStringLen.

Apropos the issue of ease of replacement, I have done this conversion once
already. I assure you it is the same amount of work to replace BSTR or CComBSTR
or whatever else is there.

[Oleksandr, 2015/01/05 11:55:40]

Let's move back to this specific code change. As I see it:

1. We do want to have RAII here.
2. We do not want to have a custom BSTR wrapper class (we've established that
earlier)

I really don't see why we can't just leave the CComBSTR and have something like:

  url = std::wstring(bstrURL, bstrURL.Length());

I agree it is a failure to move forward, in a way, but so is not 100% removal of
ATL dependencies. We have discussed this before quite extensively, I believe.

[Eric, 2015/01/05 16:18:26]

Done.
SysStringLength returns an unsigned integer, but CComBSTR::Length returns a
signed one. I've left SysStringLength in as a result.

+    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 we could change it in that
commit to '|'.

[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 by freeing the memory.

[Eric, 2014/10/20 02:36:01]

Brain fart. Forgot that this was deriving from IDispatch::Invoke.

+      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;