Issue 4395 - Setting page is loaded non-populated on some systems

Issue 4395 - Setting page is loaded non-populated on some systems.
Account for cases when the settings URL is in form:
file:///C:/Program%20Files/Adblock%20Plus%20for%20IE/html/templates/index.html
while we expect it to be in directory:
file:///C:/Program Files/Adblock Plus for IE/html/templates/

[sergei, 2016-09-07 08:23:31]

Could you please provide with an example of the path when we need it? The best
way would be to make a test with it, but just adding of it into commit
description would be also helpful.

https://codereview.adblockplus.org/29351560/diff/29351561/src/shared/Utils.cpp
File src/shared/Utils.cpp (right):

https://codereview.adblockplus.org/29351560/diff/29351561/src/shared/Utils.cp...
src/shared/Utils.cpp:214: UrlCanonicalizeW(url.c_str(), &urlCanonicalized[0],
&urlSize, NULL);
What about using of 0 instead of NULL for dwFlags here and in UrlEscape? NULL is
usually used for pointers but here the argument is a DWORD mask.

[Oleksandr, 2016-09-07 09:13:43]

Patchset 1 - use 0 instead of NULL. 

Also modified the description to show what exactly is being fixed.

https://codereview.adblockplus.org/29351560/diff/29351561/src/shared/Utils.cpp
File src/shared/Utils.cpp (right):

https://codereview.adblockplus.org/29351560/diff/29351561/src/shared/Utils.cp...
src/shared/Utils.cpp:214: UrlCanonicalizeW(url.c_str(), &urlCanonicalized[0],
&urlSize, NULL);
On 2016/09/07 08:23:31, sergei wrote:
> What about using of 0 instead of NULL for dwFlags here and in UrlEscape? NULL
is
> usually used for pointers but here the argument is a DWORD mask.

Done.

[sergei, 2016-09-07 09:15:27]

LGTM

[Eric, 2016-09-07 16:50:53]

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:160: dir = EscapeUrl(CanonicalizeUrl(dir));
There's no need to treat 'dir' as arising from a URL. It comes from a call to
system API `GetModuleFileName` and will always be in standard Window path
syntax. So at the very least calling `CanonicalizeUrl` here isn't necessary.

Besides, there's no reason to rewrite a static variable each time. If you really
want to compare in escaped URL form, just call `EscapeURL` in the
definition/initialization statement in line 158.

And in addition, I don't see why we need to compare escaped strings in the first
place. We can conver the path in the URL to canonical Windows form and compare
those. See comment in the definition of `CanonicalizeUrl` for how to do that. Or
just use the function we used to have that did that. We removed then function
`UnescapeUrl` previously because it was interfering with the blocking algorithm.
That function could be used here. It was removed in change set
5531a44442cd29020a384dc99bdddc08656e560a, and it was already fully tested.

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:161: std::wstring urlCanonicalized =
EscapeUrl(CanonicalizeUrl(url));
As a point of English usage, `urlCanonical` (with "canonical" as an adjective)
is a little better. We don't need to care how it became canonical.

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:165: log += urlCanonicalized;
It would be better here to use the argument `url` unmodified so that the log
contains what came in from the outside.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cpp
File src/shared/Utils.cpp (right):

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:212: urlCanonicalized.resize(url.length() * 2);
Canonicalization can add escape sequences, so sizing the byte buffer based only
on the input string can/will lead to buffer overflows.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:214: UrlCanonicalizeW(url.c_str(), &urlCanonicalized[0],
&urlSize, 0);
You can call this function with argument 'dwFlags' = URL_UNESCAPE to collapse
the substring "%20" to " ". (As well as any other unsafe URL characters that are
legal in path names.)

That should be sufficient to create a canonical URL for comparison.

Or we can just use the old function.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:214: UrlCanonicalizeW(url.c_str(), &urlCanonicalized[0],
&urlSize, 0);
Need to verify the return value here, just in case.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:219: std::wstring EscapeUrl(const std::wstring& url)
If we don't compare using escaped URL forms, we don't need this function at all.

[Oleksandr, 2016-09-08 02:51:16]

NOTE: Since patchset 2 was already pushed, Patchset 3 and beyond would be a
separate commit.

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
File src/plugin/PluginTabBase.cpp (right):

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:160: dir = EscapeUrl(CanonicalizeUrl(dir));
On 2016/09/07 16:50:52, Eric wrote:
> There's no need to treat 'dir' as arising from a URL. It comes from a call to
> system API `GetModuleFileName` and will always be in standard Window path
> syntax. So at the very least calling `CanonicalizeUrl` here isn't necessary.
> 
> Besides, there's no reason to rewrite a static variable each time. If you
really
> want to compare in escaped URL form, just call `EscapeURL` in the
> definition/initialization statement in line 158.
> 
> And in addition, I don't see why we need to compare escaped strings in the
first
> place. We can conver the path in the URL to canonical Windows form and compare
> those. See comment in the definition of `CanonicalizeUrl` for how to do that.
Or
> just use the function we used to have that did that. We removed then function
> `UnescapeUrl` previously because it was interfering with the blocking
algorithm.
> That function could be used here. It was removed in change set
> 5531a44442cd29020a384dc99bdddc08656e560a, and it was already fully tested.

The thinking here is that Since dir arrives from a system call we cannot assume
it to be in any format, really. I think to be safe having Canonicalization here
would be fine.

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:161: std::wstring urlCanonicalized =
EscapeUrl(CanonicalizeUrl(url));
On 2016/09/07 16:50:52, Eric wrote:
> As a point of English usage, `urlCanonical` (with "canonical" as an adjective)
> is a little better. We don't need to care how it became canonical.

Done.

https://codereview.adblockplus.org/29351560/diff/29351566/src/plugin/PluginTa...
src/plugin/PluginTabBase.cpp:165: log += urlCanonicalized;
On 2016/09/07 16:50:52, Eric wrote:
> It would be better here to use the argument `url` unmodified so that the log
> contains what came in from the outside.

During debugging I actually find it more convenient to have the processed URL
here. Just to see what exactly is being compared to what.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cpp
File src/shared/Utils.cpp (right):

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:212: urlCanonicalized.resize(url.length() * 2);
On 2016/09/07 16:50:53, Eric wrote:
> Canonicalization can add escape sequences, so sizing the byte buffer based
only
> on the input string can/will lead to buffer overflows.

Done.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:214: UrlCanonicalizeW(url.c_str(), &urlCanonicalized[0],
&urlSize, 0);
On 2016/09/07 16:50:53, Eric wrote:
> You can call this function with argument 'dwFlags' = URL_UNESCAPE to collapse
> the substring "%20" to " ". (As well as any other unsafe URL characters that
are
> legal in path names.)
> 
> That should be sufficient to create a canonical URL for comparison.
> 
> Or we can just use the old function.

Done.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:214: UrlCanonicalizeW(url.c_str(), &urlCanonicalized[0],
&urlSize, 0);
On 2016/09/07 16:50:53, Eric wrote:
> Need to verify the return value here, just in case.

Done.

https://codereview.adblockplus.org/29351560/diff/29351566/src/shared/Utils.cp...
src/shared/Utils.cpp:219: std::wstring EscapeUrl(const std::wstring& url)
On 2016/09/07 16:50:53, Eric wrote:
> If we don't compare using escaped URL forms, we don't need this function at
all.

Done.