Issue #1234 - Remove CString local variable from CPluginTabBase::InjectABP

src/plugin/PluginTabBase.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 93 matching lines @@
     filterLoaderThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
   }
   catch (const std::system_error& ex)
   {
     DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS,
       "Class::Thread - Failed to start filter loader thread");
   }
   m_traverser->ClearCache();
 }
 
+namespace
+{
+  /**
+   * Determine if the HTML file is one of ours.
+   * The criterion is that it appear in the "html/templates" folder within our installation.
+   *
+   * Warning: This function may fail if the argument is not a "file://" URL.
+   * This is occasionally the case in circumstances yet to be characterized.

[Eric, 2015/05/20 15:56:32]

I see this happen under the debugger regularly, but not even completely
consistently then. The argument, instead of a file URL, is an ordinary file
system path, complete with Windows-style backslash directory separators.

I'm not sure its actually a defect in an ordinary installation.

[sergei, 2015/06/01 09:41:30]

I see that it's sometimes in MS-DOS/Windows-style when it's called while
reacting on DownloadComplete event and it seems to be always correct URI when
it's called from DocumentComplete. JIC, the most correct reaction on document
complete is implemented in issue-119
(http://codereview.adblockplus.org/6567422169448448/).

[Eric, 2015/06/10 18:30:36]

If you'd like to add text to the comment, please propose some. Since I'm not
sure it's even a defect, I wrote this as a warning of odd behavior in case this
bit of code is ever identified as defective.

+   */
+  bool IsOurHtmlFile(std::wstring url)

[sergei, 2015/06/01 09:41:30]

const ref

[Eric, 2015/06/10 18:30:36]

Done.

+  {
+    // Declared static because the value is derived from an installation directory, which won't change during run-time.
+    static auto dir = FileUrl(HtmlFolderPath());
+    static auto dirLength = dir.length();

[sergei, 2015/06/01 09:41:30]

I would say we don't need `dirLength` variable.

[Eric, 2015/06/10 18:30:36]

Done.

+
+    std::wstring log = L"InjectABP. Current URL: ";
+    log += url;
+    log += L", template directory URL: ";
+    log += dir;
+    DEBUG_GENERAL(log);
+
+    auto urlCstr = url.c_str();

[sergei, 2015/06/01 09:41:30]

Actually, this variable is also not necessary.

[Eric, 2015/06/10 18:30:36]

Not strictly, no; it's a small optimization. But eliminating it means calling
c_str() twice down below.

+    // Check the prefix to match our directory
+    // Check the suffix to be an HTML file
+    return (_wcsnicmp(urlCstr, dir.c_str(), dirLength) == 0) &&
+      (_wcsnicmp(urlCstr + url.length() - 5, L".html", 5) == 0);

[sergei, 2015/06/01 09:41:30]

It would be great to have a comment here explaining the reason the length of
`url` is always not less than 5.

[Eric, 2015/06/10 18:30:36]

We actually need an explicit check, which I've added.

I got myself confused before ('dir' is always of length at least 15) and didn't
have it in the first place.

+  }
+}
+
 void CPluginTabBase::InjectABP(IWebBrowser2* browser)
 {
   CriticalSection::Lock lock(m_csInject);
   auto url = GetDocumentUrl();
-
-  std::wstring log = L"InjectABP. Current URL: ";
-  log += url;
-  log += L", settings URL: ";
-  log += UserSettingsFileUrl();
-  DEBUG_GENERAL(log);
-
-  CString urlLegacy = ToCString(url);
-  if (!(0 == urlLegacy.CompareNoCase(CString(UserSettingsFileUrl().c_str())) ||
-      0 == urlLegacy.CompareNoCase(CString(FirstRunPageFileUrl().c_str()))))
+  if (!IsOurHtmlFile(url))
   {
-    DEBUG_GENERAL(L"Not injecting");
+    DEBUG_GENERAL(L"InjectABP. Not injecting");
     return;
   }
-  DEBUG_GENERAL(L"Going to inject");
+  DEBUG_GENERAL(L"InjectABP. Injecting");
   CComPtr<IDispatch> pDocDispatch;
   browser->get_Document(&pDocDispatch);
   CComQIPtr<IHTMLDocument2> pDoc2 = pDocDispatch;
   if (!pDoc2)
   {
     DEBUG_ERROR_LOG(0, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::InjectABP - Failed to QI document");
     return;
   }
   CComPtr<IHTMLWindow2> pWnd2;
   pDoc2->get_parentWindow(&pWnd2);
@@ skipping 216 matching lines @@
           LogQueue::LogPluginError(pluginError.GetErrorCode(), pluginError.GetErrorId(), pluginError.GetErrorSubid(), pluginError.GetErrorDescription(), true, pluginError.GetProcessId(), pluginError.GetThreadId());
         }
 
         // Non-hanging sleep
         Sleep(50);
       }
 
       tabLoopIteration++;
   }
 }