Issue #1234 - Convert strings associated with URL's

Issue #1234 - Convert strings associated with URL's

The focus on this change is to convert strings that represent URL's from
ATL::CString to std::wstring, especially when such strings appear as
arguments in function signatures.

Removed CString version UnescapeUrl.

Left unconverted was one case where we don't yet have an equivalent CString
member function.

[sergei, 2014-10-17 10:10:12]

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/AdblockPlusDomTraverser.cpp:55: SysFreeString(vAttr.bstrVal);
It's not necessary to call SysFreeString here because ATL::CComVariant cares
about it.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:211: BSTR bstrURL;
Why is ATL::CComBSTR replaced by BSTR? Looks like degradation.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:551: auto arg = pDispParams->rgvarg[5];
I would prefer `const auto&` here.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:553: if (vt == VT_BYREF + VT_VARIANT &&
arg.pvarVal->vt == VT_BSTR)
'+' here looks unusual, despite it works, I think we could change it in that
commit to '|'.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:558: SysFreeString(b);
It's not allowed to modify arguments, especially by freeing the memory.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:725: BSTR bstrUrl;
Again CComBSTR is replaced by BSTR.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginDebug.cpp:188: CString srcTrunc = ToCString(src);
Despite it might be not a part of this change, I would refactor it to a separate
function now.
std::wstring shorten(const std::wstring& value)
{
  if (value.length() > 100)
  {
    return value.substr(0, 67) + L"..." + value.substr(src.length() - 30 - 1);
  }
  return value;
}

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginDebug.cpp:201: void CPluginDebug::DebugResultHiding(const
CString& tag, const CString& id, const CString& filter)
Why is it renamed to id?

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:225: BSTR bstrSrc;
Again CComBSTR is replaced by BSTR.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginWbPassThrough.cpp:48: int
WBPassthruSink::GetContentTypeFromURL(std::wstring src)
`const std::wstring&`

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginWbPassThrough.cpp:94: int
WBPassthruSink::GetContentType(CString mimeType, std::wstring domain,
std::wstring src)
const ATL::CString&, const std::wstring&

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginWbPassThrough.h:21: int GetContentType(CString mimeType,
std::wstring domain, std::wstring src);
see comments in cpp file

[Eric, 2014-10-20 02:36:01]

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/AdblockPlusDomTraverser.cpp:55: SysFreeString(vAttr.bstrVal);
On 2014/10/17 10:10:12, sergei wrote:
> It's not necessary to call SysFreeString here because ATL::CComVariant cares
> about it.

True. Removed it.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:211: BSTR bstrURL;
On 2014/10/17 10:10:12, sergei wrote:
> Why is ATL::CComBSTR replaced by BSTR? Looks like degradation.

Because we don't need CComBSTR here. The only reason to use it is to call
SysFreeString. Eliminating it improves the code by showing explicitly what the
BSTR needs. There's no mysterious secret hiding within CComBSTR.

CComBSTR doesn't convert naturally to std::wstring like our own class would,
either. Having such a conversion improves the readability of the code
significantly (I know because I've already done it once). Keeping the shroud of
mystery around CComBSTR just keeps the code from moving forward.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:551: auto arg = pDispParams->rgvarg[5];
On 2014/10/17 10:10:12, sergei wrote:
> I would prefer `const auto&` here.

OK.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:553: if (vt == VT_BYREF + VT_VARIANT &&
arg.pvarVal->vt == VT_BSTR)
On 2014/10/17 10:10:12, sergei wrote:
> '+' here looks unusual, despite it works, I think we could change it in that
> commit to '|'.

It's out of scope, but it's also trivial.

Done.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:558: SysFreeString(b);
On 2014/10/17 10:10:12, sergei wrote:
> It's not allowed to modify arguments, especially by freeing the memory.

Brain fart. Forgot that this was deriving from IDispatch::Invoke.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:725: BSTR bstrUrl;
On 2014/10/17 10:10:12, sergei wrote:
> Again CComBSTR is replaced by BSTR.

And for the same reasons I stated above.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginDebug.cpp:188: CString srcTrunc = ToCString(src);
On 2014/10/17 10:10:12, sergei wrote:
> Despite it might be not a part of this change

Out of scope for this review.

The whole debug logging system could use an overhaul. It's using CString::Format
instead of the standard library format functions. There's already a rewrite
scheduled as part of the process to convert away from CString.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginDebug.cpp:201: void CPluginDebug::DebugResultHiding(const
CString& tag, const CString& id, const CString& filter)
On 2014/10/17 10:10:12, sergei wrote:
> Why is it renamed to id?

That's how it is declared in the header, which better reflects how it's used in
the code. It's only used for certain attributes (class names and element
identifiers) and not for URL's.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginWbPassThrough.cpp:48: int
WBPassthruSink::GetContentTypeFromURL(std::wstring src)
On 2014/10/17 10:10:12, sergei wrote:
> `const std::wstring&`

Done.

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginWbPassThrough.cpp:94: int
WBPassthruSink::GetContentType(CString mimeType, std::wstring domain,
std::wstring src)
On 2014/10/17 10:10:12, sergei wrote:
> const ATL::CString&, const std::wstring&

Changed the std::wstring arguments. The CString one is scheduled for conversion
soon.

[sergei, 2014-10-21 09:45:17]

Beside the comments, I think we should rabase it after pushing Oleksandr's patch
with the idea in http://codereview.adblockplus.org/4974480757620736/ because
PluginWbPassThrough.cpp has already conflicts and it will conflict again with
that patch.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/AdblockPlusDomTraverser.cpp:55: SysFreeString(vAttr.bstrVal);
On 2014/10/20 02:36:01, Eric wrote:
> On 2014/10/17 10:10:12, sergei wrote:
> > It's not necessary to call SysFreeString here because ATL::CComVariant cares
> > about it.
> 
> True. Removed it.

But it's still here.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:211: BSTR bstrURL;
On 2014/10/20 02:36:01, Eric wrote:
> On 2014/10/17 10:10:12, sergei wrote:
> > Why is ATL::CComBSTR replaced by BSTR? Looks like degradation.
> 
> Because we don't need CComBSTR here. The only reason to use it is to call
> SysFreeString. Eliminating it improves the code by showing explicitly what the
> BSTR needs. There's no mysterious secret hiding within CComBSTR.
> 
> CComBSTR doesn't convert naturally to std::wstring like our own class would,
> either. Having such a conversion improves the readability of the code
> significantly (I know because I've already done it once). Keeping the shroud
of
> mystery around CComBSTR just keeps the code from moving forward.

Sorry, I don't understand about which mystery your talking.
The main rule is to use RAII, ATL::CComBSTR completely satisfies it and it's
completely enough for us in my personal opinion. Using manual resource
management is a step backward.
What concerns using our own wrapper around BSTR. First of all we should ask
whether we need it or not, I am not convinced that we need it. Secondly, to
replace ATL::CComBSTR by it seems easier than to replace
BSTR+SysStringLen+SysFreeString

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginDebug.cpp:201: void CPluginDebug::DebugResultHiding(const
CString& tag, const CString& id, const CString& filter)
On 2014/10/20 02:36:01, Eric wrote:
> On 2014/10/17 10:10:12, sergei wrote:
> > Why is it renamed to id?
> 
> That's how it is declared in the header, which better reflects how it's used
in
> the code. It's only used for certain attributes (class names and element
> identifiers) and not for URL's.

In CPluginDomTraverser::HideElement we pass url, but nevertheless 'id' looks
reasonable here.

http://codereview.adblockplus.org/4912420225024000/diff/5676830073815040/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5676830073815040/src/...
src/plugin/PluginWbPassThrough.h:20: int GetContentTypeFromURL(std::wstring&
src);
Why is it not const here and below?

[Eric, 2014-10-21 17:19:51]

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/AdblockPlusDomTraverser.cpp:55: SysFreeString(vAttr.bstrVal);
On 2014/10/21 09:45:17, sergei wrote:
> But it's still here.

I _thought_ I had removed it.

It's gone now.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:211: BSTR bstrURL;
On 2014/10/21 09:45:17, sergei wrote:
> Sorry, I don't understand about which mystery your talking.

That's because there is no mystery.

I take it, then, that you do understand that all we need here is to call
SysFreeString in a destructor, whether that be in CComBSTR or in some other
wrapper class.

> The main rule is to use RAII, ATL::CComBSTR completely satisfies it and it's
> completely enough for us in my personal opinion. Using manual resource
> management is a step backward.

I agree about RAII, but CComBSTR is not the only way of accomplishing it.

Continuing to use CComBSTR is failing to step forward. We did not decide to
enshrine ATL; we decided to de-prioritize removing it.

> What concerns using our own wrapper around BSTR. First of all we should ask
> whether we need it or not, I am not convinced that we need it.

Somehow, even though I keep repeating them, I haven't seen you acknowledge that
there are advantages to using our own wrapper class. We haven't even got to the
point where we are able to weigh advantages vs. disadvantages. 

So, once more, there are several advantages to having our own wrapper class for
this case, namely, [in] arguments on the client side:

1) Built-in conversion operator to std::wstring().
  a) Without such an operator, each and every conversion requires
    i) A condition to check that the BSTR value (raw or wrapped) is not null,
and
    ii) An explicit call to SysStringLen(), because BSTR are not guaranteed to
be
        null-terminated.

2) We can make all the tests for empty strings consistent by calling 'empty()'.
  a) The code at present does not
  
3) Its constructor can be declared 'noexcept', since it doesn't allocate.
  a) Don't expect Microsoft to update ATL for C++11.

4) Throw an exception for erroneous uses of 'operator&'.
  a) The ATL version does not do so in Release configurations
  b) We can throw 'std::logic_error' instead of the non-'std' ATL exception.

> Secondly, to
> replace ATL::CComBSTR by it seems easier than to replace
> BSTR+SysStringLen+SysFreeString

If we were to use CComBSTR, we cannot replace SysStringLen. BSTR is not
guaranteed to be null-terminated, so we cannot use the simpler constructor for
'wstring'. That means that if we enshrine CComBSTR, we enshrine explicit calls
to SysStringLen.

Apropos the issue of ease of replacement, I have done this conversion once
already. I assure you it is the same amount of work to replace BSTR or CComBSTR
or whatever else is there.

http://codereview.adblockplus.org/4912420225024000/diff/5676830073815040/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5676830073815040/src/...
src/plugin/PluginWbPassThrough.h:20: int GetContentTypeFromURL(std::wstring&
src);
On 2014/10/21 09:45:17, sergei wrote:
> Why is it not const here and below?

Because I'm focusing only on type conversion. I have not also been checking
whether the arguments could be declare 'const'.

I've added the declarations.

[Oleksandr, 2015-01-05 11:55:40]

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:211: BSTR bstrURL;
Let's move back to this specific code change. As I see it:

1. We do want to have RAII here.
2. We do not want to have a custom BSTR wrapper class (we've established that
earlier)

I really don't see why we can't just leave the CComBSTR and have something like:

  url = std::wstring(bstrURL, bstrURL.Length());

I agree it is a failure to move forward, in a way, but so is not 100% removal of
ATL dependencies. We have discussed this before quite extensively, I believe.

http://codereview.adblockplus.org/4912420225024000/diff/5643440998055936/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/4912420225024000/diff/5643440998055936/src/...
src/plugin/PluginFilter.cpp:676: srcCString.Trim();
We have TrimString function in shared/Utils.h for this.

[Eric, 2015-01-05 16:18:26]

New patch set has been rebased.

I added one more conversion, which provided the opportunity to get rid of the
CString version of UnescapeUrl in this patch set.

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

http://codereview.adblockplus.org/4912420225024000/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:211: BSTR bstrURL;
On 2015/01/05 11:55:40, Oleksandr wrote:
> I really don't see why we can't just leave the CComBSTR

Done.

>  url = std::wstring(bstrURL, bstrURL.Length());

SysStringLength returns an unsigned integer, but CComBSTR::Length returns a
signed one. I've left SysStringLength in as a result.

http://codereview.adblockplus.org/4912420225024000/diff/5643440998055936/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/4912420225024000/diff/5643440998055936/src/...
src/plugin/PluginFilter.cpp:676: srcCString.Trim();
On 2015/01/05 11:55:40, Oleksandr wrote:
> We have TrimString function in shared/Utils.h for this.

Done.

[sergei, 2015-01-06 13:18:17]

LGTM, except two not important comments.

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
src/plugin/PluginDomTraverserBase.h:277: srcLegacy = L"http://" +
to_CString(m_domain) + srcLegacy;
should ToCString be instead of to_CString here?

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
src/plugin/PluginWbPassThrough.h:23: int GetContentTypeFromURL(const
std::wstring& src);
We use tabs in this file, but here are spaces, it's inconsistent.

I think the file should be converted, but I not in this changeset.

[Eric, 2015-01-06 19:38:59]

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
src/plugin/PluginDomTraverserBase.h:277: srcLegacy = L"http://" +
to_CString(m_domain) + srcLegacy;
On 2015/01/06 13:18:18, sergei wrote:
> should ToCString be instead of to_CString here?

It's time to do a wholesale replacement, which was the idea all along. After
this change set lands, I'll get to it.

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
File src/plugin/PluginWbPassThrough.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
src/plugin/PluginWbPassThrough.h:23: int GetContentTypeFromURL(const
std::wstring& src);
On 2015/01/06 13:18:18, sergei wrote:
> We use tabs in this file, but here are spaces, it's inconsistent.

Even more than inconsistent, the existing code violates the indentation rule of
our style guide.

It's true, replacing all the tabs deserves its own change.

[Oleksandr, 2015-01-13 10:08:06]

LGTM, but please replace to_Cstring for ToCString asap.

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/4912420225024000/diff/5707274949492736/src/...
src/plugin/PluginDomTraverserBase.h:277: srcLegacy = L"http://" +
to_CString(m_domain) + srcLegacy;
Would be great to not commit to_CString anyway though. Since we have a correctly
styled method anyway.

[Eric, 2015-01-13 16:46:48]

On 2015/01/13 10:08:06, Oleksandr wrote:
> LGTM, but please replace to_Cstring for ToCString asap.

See http://codereview.adblockplus.org/5491870822039552/