Issue #1234 - Replace CString in the traverser

Issue #1234 - Replace CString in the traverser

Change all the 'CString' arguments in traverser functions to 'std::wstring'.
Add 'const' declaration to all the 'indent' arguments.

Extract 'CString::MakeLower' into a utility function 'ToLowerString()'. Defer
rewriting this function to avoid 'MakeLower' pending resolution of locale
questions.

Rewrite loop for <object> elements to eliminate 'CString' local variables.

[Eric, 2015-12-07 16:12:22]

This is the last of the major changes required for #1234. There will be one more
final cleanup one.

[sergei, 2015-12-08 11:34:21]

LGTM

[Eric, 2015-12-09 13:20:03]

Added new review https://codereview.adblockplus.org/29332484/ to avoid a
dependency on a larger review under discussion. Added dependency information to
the review description.

[Eric, 2015-12-14 12:04:45]

Recent commit of ToWstring(BSTR) change removes dependency on other reviews.
Changed description accordingly.

New patch set is a rebase to current tip. The change set was previously based on
a change set in another review. Other than difference in patch context, its only
difference from the original patch set is to remove changes in PluginFilter.cpp,
which are not needed when this change set stands alone.

[sergei, 2015-12-14 12:17:55]

LGTM

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
File src/plugin/AdblockPlusDomTraverser.cpp (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
src/plugin/AdblockPlusDomTraverser.cpp:93: posBegin =
objectInnerHtml.find(L"VALUE=\"", posBegin);
It's not related to this issue, but shouldn't we perform case insensitive search
for "value" attribute?

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginUt...
File src/plugin/PluginUtil.h (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginUt...
src/plugin/PluginUtil.h:36: * Current version uses 'CString::MakeLower()' to
preserve legacy behavior during refactoring.
I'm not sure about the latter line. It might be better to move it into the
function body, so it explains the code.

[Eric, 2015-12-14 12:40:24]

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
File src/plugin/AdblockPlusDomTraverser.cpp (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
src/plugin/AdblockPlusDomTraverser.cpp:93: posBegin =
objectInnerHtml.find(L"VALUE=\"", posBegin);
On 2015/12/14 12:17:55, sergei wrote:
> It's not related to this issue, but shouldn't we perform case insensitive
search
> for "value" attribute?

If we change it, we shouldn't be parsing the innerHTML attribute at all. The
"value" attribute should be appearing on a <param> tag inside the present
<object> tag. It would be more reliable to use DOM calls here.

When I saw this, I assumed that IE must be normalizing the innerHTML of <object>
elements. Given that we don't have a rash of reports of not blocking Flash etc.,
I'm assuming that this behavior is consistent.

I'm not inclined to change this, since we have bigger issues to worry about. If
you'd like to create a ticket for it, though, it does seem like a valid concern.

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginUt...
File src/plugin/PluginUtil.h (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginUt...
src/plugin/PluginUtil.h:36: * Current version uses 'CString::MakeLower()' to
preserve legacy behavior during refactoring.
On 2015/12/14 12:17:55, sergei wrote:
> I'm not sure about the latter line. It might be better to move it into the
> function body, so it explains the code.

It's here not to explain the code, but to explain the behavior. This says that
the lowercase algorithm is the one used by CString, as opposed to, say, an
ASCII-only lowercase algorithm.

[Oleksandr, 2015-12-14 12:45:34]

LGTM

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
File src/plugin/AdblockPlusDomTraverser.cpp (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
src/plugin/AdblockPlusDomTraverser.cpp:93: posBegin =
objectInnerHtml.find(L"VALUE=\"", posBegin);
On 2015/12/14 12:17:55, sergei wrote:
> It's not related to this issue, but shouldn't we perform case insensitive
search
> for "value" attribute?

It wouldn't hurt, but it doesn't depend on how it is written in the HTML source.
get_innerHTML normalizes HTML for us.

[Oleksandr, 2015-12-14 12:49:07]

Sorry, Not LGTM yet. Missed one important change.

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginDo...
File src/plugin/PluginDomTraverserBase.h (left):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginDo...
src/plugin/PluginDomTraverserBase.h:113: void
CPluginDomTraverserBase<T>::TraverseSubdocument(IWebBrowser2* pBrowser, const
std::wstring& domain, const CString& documentName)
We still need this parameter. I would be better of if it was renamed to
documentUrl, but we are using it in https://codereview.adblockplus.org/29331669/
for example.

[sergei, 2015-12-14 13:28:03]

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
File src/plugin/AdblockPlusDomTraverser.cpp (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
src/plugin/AdblockPlusDomTraverser.cpp:93: posBegin =
objectInnerHtml.find(L"VALUE=\"", posBegin);
On 2015/12/14 12:40:23, Eric wrote:
> On 2015/12/14 12:17:55, sergei wrote:
> > It's not related to this issue, but shouldn't we perform case insensitive
> search
> > for "value" attribute?
> 
> If we change it, we shouldn't be parsing the innerHTML attribute at all. The
> "value" attribute should be appearing on a <param> tag inside the present
> <object> tag. It would be more reliable to use DOM calls here.
Yes, I thought about it too. The current approach seems to be a result of trade
between something simple and enough good working, and more sophisticated proper
solution but without real gain.
> 
> When I saw this, I assumed that IE must be normalizing the innerHTML of
<object>
> elements. Given that we don't have a rash of reports of not blocking Flash
etc.,
> I'm assuming that this behavior is consistent.
>
> I'm not inclined to change this, since we have bigger issues to worry about.
If
> you'd like to create a ticket for it, though, it does seem like a valid
concern.
>

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
src/plugin/AdblockPlusDomTraverser.cpp:93: posBegin =
objectInnerHtml.find(L"VALUE=\"", posBegin);
On 2015/12/14 12:45:34, Oleksandr wrote:
> On 2015/12/14 12:17:55, sergei wrote:
> > It's not related to this issue, but shouldn't we perform case insensitive
> search
> > for "value" attribute?
> 
> It wouldn't hurt, but it doesn't depend on how it is written in the HTML
source.
> get_innerHTML normalizes HTML for us.

Could you provide with a reference for it? I've tried it in javascript and could
not reproduce automatic conversion to uppercase of innerHTML property.

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginUt...
File src/plugin/PluginUtil.h (right):

https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/PluginUt...
src/plugin/PluginUtil.h:36: * Current version uses 'CString::MakeLower()' to
preserve legacy behavior during refactoring.
On 2015/12/14 12:40:23, Eric wrote:
> On 2015/12/14 12:17:55, sergei wrote:
> > I'm not sure about the latter line. It might be better to move it into the
> > function body, so it explains the code.
> 
> It's here not to explain the code, but to explain the behavior. This says that
> the lowercase algorithm is the one used by CString, as opposed to, say, an
> ASCII-only lowercase algorithm.

Acknowledged.

[Eric, 2015-12-14 13:28:53]

New patch set addresses Oleksandr's concern. 
- Added (currently unused) documentUrl argument to TraverseSubdocument(). 
- Renamed analogous argument (also currently unused) in TraverseDocument()
because why not?

[Oleksandr, 2015-12-14 14:01:31]

>
https://codereview.adblockplus.org/29332057/diff/29332595/src/plugin/AdblockP...
> src/plugin/AdblockPlusDomTraverser.cpp:93: posBegin =
> objectInnerHtml.find(L"VALUE=\"", posBegin);
> On 2015/12/14 12:45:34, Oleksandr wrote:
> > On 2015/12/14 12:17:55, sergei wrote:
> > > It's not related to this issue, but shouldn't we perform case insensitive
> > search
> > > for "value" attribute?
> > 
> > It wouldn't hurt, but it doesn't depend on how it is written in the HTML
> source.
> > get_innerHTML normalizes HTML for us.
> 
> Could you provide with a reference for it? I've tried it in javascript and
could
> not reproduce automatic conversion to uppercase of innerHTML property.
> 
Actually looks like you're right. I have just tested in the API itself and IE is
now giving non-normalized string. I have created an issue for this here:
https://issues.adblockplus.org/ticket/3409.

[Oleksandr, 2015-12-14 14:03:50]

LGTM. Thanks.