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

src/plugin/PluginTabBase.cpp

Index: src/plugin/PluginTabBase.cpp
===================================================================
--- a/src/plugin/PluginTabBase.cpp
+++ b/src/plugin/PluginTabBase.cpp
@@ -111,25 +111,45 @@
   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;