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

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

A local copy of the argument 'url' had been used for the case-insensitive
string comparison member function of CString. Factor out the predicate into
its own function and rewrite it to compare with 'wcsnicmp()'.

Also, the predicate was changed to recognize any ".html" file in the
"html/templates" folder within our installation, rather than just the two
files explicitly enumerated before.

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

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:121: * This is occasionally the case in
circumstances yet to be characterized.
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]

It's more than merely getting rid of ATL::CString, there is also an important
change in the logic. I don't want to look too bureaucratic, but may be it's
worth additional issue for that.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:121: * This is occasionally the case in
circumstances yet to be characterized.
On 2015/05/20 15:56:32, Eric wrote:
> 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.

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/).

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:123: bool IsOurHtmlFile(std::wstring url)
const ref

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:127: static auto dirLength = dir.length();
I would say we don't need `dirLength` variable.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:135: auto urlCstr = url.c_str();
Actually, this variable is also not necessary.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:139: (_wcsnicmp(urlCstr + url.length() - 5,
L".html", 5) == 0);
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]

New patch set, rebased to current public tip.

On 2015/06/01 09:41:30, sergei wrote:
> I don't want to look too bureaucratic, but may be it's
> worth additional issue for that.

An additional issue would indeed be too bureaucratic. I've expanded the commit
message so that the change is more visible.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:121: * This is occasionally the case in
circumstances yet to be characterized.
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.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:123: bool IsOurHtmlFile(std::wstring url)
On 2015/06/01 09:41:30, sergei wrote:
> const ref

Done.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:127: static auto dirLength = dir.length();
On 2015/06/01 09:41:30, sergei wrote:
> I would say we don't need `dirLength` variable.

Done.

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:135: auto urlCstr = url.c_str();
On 2015/06/01 09:41:30, sergei wrote:
> Actually, this variable is also not necessary.

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

http://codereview.adblockplus.org/5670602698391552/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:139: (_wcsnicmp(urlCstr + url.length() - 5,
L".html", 5) == 0);
On 2015/06/01 09:41:30, sergei wrote:
> It would be great to have a comment here explaining the reason the length of
> `url` is always not less than 5.

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.

[sergei, 2015-10-02 10:04:57]

LGTM

[Oleksandr, 2015-10-05 10:15:04]

LGTM