[IE] First round of ATL removal

src/plugin/COM_Client.h

Index: src/plugin/COM_Client.h
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/plugin/COM_Client.h
@@ -0,0 +1,120 @@
+

[Oleksandr, 2014/06/26 00:48:43]

I guess COM_Client.h is not needed?

[Eric, 2014/06/26 15:13:56]

I take it you mean COM_Client.cpp.

Right now, all that I've got so far is these parameter-marshalling classes,
which are simple enough to make inline. The implementations for the other COM
support classes won't be so simple, and I anticipate I'll be using the file for
that.

+/**
+ * Wrapper for BSTR values returned by COM components.
+ *
+ * The term "values returned" here means "out" parameters to COM interfaces.
+ * A number of IBrowser functions return allocated BSTR values this way, 
+ *   values that require the caller to free them.
+ * ATL provides CComBSTR for these return values.
+ * CComBSTR, however, implements a number of general purpose string functions,
+ *   but the present one does not.
+ *
+ * The sole responsibility of this class is to manage the allocated resource.
+ * The supported pattern of use is exactly two steps:
+ *   -# Construct an instance with the BSTR value returned from the COM call
+ *   -# Copy the contents to a std::wstring.
+ * There generally no reason even to declare a separate variable of this type,
+ *   since the value of the conversion operator can be assigned directly.
+ *
+ * This class does no error checking for null pointer values.
+ * There is, however, no default constructor to initialize one easily.
+ * The only constructor takes a BSTR argument,
+ *   so the caller has the opportunity to test the value of the "out" argument.
+ */
+class Returned_BSTR

[Oleksandr, 2014/06/26 00:48:43]

This multiple classes approach is a bit confusing, IMO. Why not have one class
for all cases? I don't think an extra copy operation makes a difference.

For example something like below (NOTE: not tested) 

class BSTR_AdblockPlus
{
  BSTR m_str;
  BSTR_AdblockPlus(wchar_t* b)
  {
    m_str = ::SysAllocStringLen(s.c_str(), s.length());
  }
  BSTR_AdblockPlus(BSTR b)
  {
    m_str = ::SysAllocStringByteLen((char*)b, ::SysStringByteLen(b));
    ::SysFreeString(b);
  }
  ~BSTR_AdblockPlus()
  {
    ::SysFreeString(m_str);
  }

...


}

[Eric, 2014/06/26 15:13:56]

The biggest confusion to my eye is the naming. I don't particularly like these
names, but I didn't know what was needed when I started. I'm planning to
rationalize them at the end of this process. It looks like we have four cases,
each with slightly different lifecycle requirements:

1) As caller: outgoing arguments to external functions
2) As caller: incoming return values from external functions
3) As callee: incoming arguments to our functions
4) As callee: outgoing return values from our functions

I'll move to a more consistent naming convention when I rationalize all the
wrapper functions. That'll be at the end of the ATL removal.
Extraneous memory copying is one of the biggest performance penalties in
software generally. It's something to pay attention to here, since there are
lots of IE and DOM calls that use BSTR. We're already taking a performance hit
by using std::wstring and copying to BSTR, but without a basic_string
implementations that's known also to be a BSTR, that's what we're stuck with.

For the record, I think the maintenance advantages of using standard strings
outweigh the performance disadvantages.

[sergei, 2014/07/08 11:58:34]

I also would like to have only one class.
As well in COM it's a common practice to have `operator&` which returns the
valid pointer if the internal state is "empty".

One thing which should be taken into the account designing such classes is to be
exception friendly.
Compare the cases:
BSTR result;
getResult(&result);
some code // which throws an exception
if (some condition) // condition is false, who will free result?
{
  value = result ? Returned_BSTR( result ) : std::wstring();
}

vs

AnotherSmartBSTR result; // we can exit from the function at any point, the
result will be freed.
getResult(&result);
some code // which throws an exception
std::wstring value = result;

Another thing here is the absence of copy-ctr, assignment operator and for C++11
a couple of members with the move semantics. If you don't want to fiddle around
with them I would like to use `std::unique_ptr` with custom deleter.

On 2014/06/26 15:13:56, Eric wrote:

[Eric, 2014/07/08 17:46:37]

Here's the place for the longer rant about address-of and COM. The way CComPtr
is implemented, it returns a writable pointer-to-pointer in all cases, not just
when the pointer is zero, but only in Release configurations, because in Debug
there's an assertion that will catch it. In other words, it does not actually
enforce its own invariant. Since, however, it catches the most obvious defective
usages in the development cycle, all that's left are the rare and unusual ones.
This is pretty much the worst case possible.

That said, it's possible to write an efficient and correct class that has
'operator&'. I just didn't do it in this case. This class was written before I
had much experience with what the right pattern is for this kind of type
conversion interface for this class of system calls. I know how to do it better
now.

I'm putting it on my to-do list to rewrite this class when I work on the IE
wrapper classes next.
Yes, the overall point is well-taken. I should point out, however, that the
present code doesn't leak memory, since all the uses of this class have
intervening code that can't throw. (I just verified this.)
For this class, whose sole purpose is to receive-and-convert, all those
operators should be declared "= delete", which VS 2012 doesn't support. (For the
record: copy constructor, move constructor, copy assignment, and move
assignment. None are used presently and none should be used.)

+{
+  /**
+   * The underlying BSTR variable.
+   */
+  wchar_t * _bstr ;
+
+public:
+  /**
+   * Ordinary constructor.
+   */
+  Returned_BSTR( wchar_t * b )
+    : _bstr( b )
+  {}
+
+  ~Returned_BSTR()
+  {
+    ::SysFreeString( _bstr );
+  }
+
+  operator std::wstring()
+  {
+    return std::wstring( _bstr, ::SysStringLen( _bstr )  );
+  }
+};
+
+class Invoke_Result
+{
+  VARIANT & _result;
+
+public:
+  /**
+   * Ordinary constructor.
+   *
+   * The caller has responsibility for checking that the result pointer argument in Invoke() is not null.
+   * Thus the argument is a reference, not a pointer, to ensure that the caller fulfills its responsibility.
+   */
+  Invoke_Result( VARIANT & result )
+    : _result( result )
+  {}
+
+  void assign_wstring( std::wstring x )
+  {
+    _result.vt = VT_BSTR;
+    _result.bstrVal = SysAllocString( x.c_str() );
+  }
+};
+
+inline void invoke_result_wstring( VARIANT & result, std::wstring s )
+{
+  Invoke_Result ir( result );
+  ir.assign_wstring( s );
+}
+
+class BSTR_Argument
+{
+  BSTR _bstr;
+public:
+  BSTR_Argument( std::wstring s )
+    : _bstr( SysAllocStringLen( s.c_str(), s.length() ) )
+  {
+    // Preserve invariant that _bstr is not null
+    if ( !_bstr )
+    {
+      // Assert SysAllocStringLen return null, meaning out of memory
+      throw std::bad_alloc();
+    }
+  }
+
+  ~BSTR_Argument()
+  {
+    SysFreeString( _bstr );
+  }
+    
+  inline operator BSTR()
+  {
+    return _bstr;
+  }
+};
+
+/**
+ * Constructor class for incoming BSTR argument for dispatch entry points.
+ *
+ * The BSTR coming in as arguments from dispatch calls are allocated by the caller.
+ * We don't need to manage the lifetime of these arguments.
+ * Since we are immediately converting to wstring, we don't need to allocate our own BSTR.
+ * Thus, even though the constructor pattern looks like BSTR_Argument, 
+ *   we don't want the allocate/free behavior of its implementation.
+ */
+class Incoming_BSTR
+  : public std::wstring
+{
+public:
+  Incoming_BSTR( BSTR bstr )
+    : std::wstring( bstr, SysStringLen( bstr ) )
+  {}
+};