Issue #41 - Bring method of determining IE version up to date

src/shared/Registry.cpp

 #include "Registry.h"
 #include <memory>
 
 using namespace AdblockPlus;
 
-Registry_Key::Registry_Key(HKEY parent, std::wstring key_name)
+RegistryKey::RegistryKey(HKEY parent, const std::wstring& key_name)
 {
   if (key_name.empty())
   {
     throw std::runtime_error("key_name may not be empty");
   }
   HRESULT hr = RegOpenKeyExW(parent, key_name.c_str(), 0, KEY_QUERY_VALUE, &key);
   if (hr != ERROR_SUCCESS || !key)
   {
     throw std::runtime_error("Failure in RegOpenKeyExW");
   }
 }
 
-Registry_Key::~Registry_Key()
+RegistryKey::~RegistryKey()
 {
   RegCloseKey(key);
 }
 
-std::wstring Registry_Key::value_wstring(std::wstring name) const
+std::wstring RegistryKey::value_wstring(const std::wstring& name) const
 {
   /*
    * Step one is to determine the presence of the value, along with its type and byte size.
    */
   DWORD type;
   DWORD size = 0;
   HRESULT hr = ::RegQueryValueExW(static_cast<HKEY>(key), name.c_str(), 0, &type, 0, &size);
   if (hr != ERROR_SUCCESS)
   {
     throw std::runtime_error("Failure in RegQueryValueEx to query name");

[sergei, 2014/07/28 11:46:27]

It's not descriptive. Is it possible to find a root of problem if there is a
long list or only several entries of "Failure in RegQueryValueEx to query name"
in the log? Include at least the `name` in the message, as well as the error
code. For now it will be better to facilitate system_error, despite
`std::system_category` is not properly implemented by MS and we need to have our
own impl. It will be easier to find and fix all cases just looking for
`std::system_category`.

[Eric, 2014/07/29 12:42:25]

No, it's not, and it doesn't really matter at the moment. The propagation
exceptions here are all truncated in the interface functions and their content
dropped. So it matters little what these exceptions are at the moment. The main
reason I didn't just use std::exception() is to provide a bit of documentation
for the debugger.

It would be premature at this moment to over-engineer these exceptions. We need
three things in place to make the worthwhile: (1) try-catch{...) at all entry
points (2) consistent behavior in the catch-all blocks to at least log the
exception (including field-installed Release configurations), and (3) our own
exception classes that provide consistent information. When we've got these, it
will straightforward to update all the exceptions in the code, including these.

[sergei, 2014/07/29 14:45:56]

It does always matter.
- There is no real reason to drop the error message.
- It is very useful for postmortem investigation as well as for everyday
debugging. It allows to catch problem earlier.
- if we use the unified and regular error handling there is no problem or
difficulties with it, except the laziness.

[Eric, 2014/07/29 15:17:22]

Yes, and when we get unified and regular error handling, then it will be time to
modify this.

You are making an implicit claim that simply using system_error will be
sufficient, and I don't agree with that. This code will also end up in the
installer custom action code (see #404 yet again), not just in the engine and
the plugin, and we do not have any shared error handling yet that can simply be
ported over there easily yet.

[sergei, 2014/07/30 10:42:04]

I'm pretty sure that std::system_error can be used in the installer as well
because it inherits std::runtime_error.

[Eric, 2014/07/30 13:02:04]

It's not the exception that's the main problem, it's the lack of general-purpose
code to catch it and, at the least, log it. If we don't have that, it doesn't
matter at all what you throw. And, as I pointed out elsewhere in the present set
of comments, we don't have such general-purpose code yet, much less in a library
available to the installer.

Furthermore, If we want to identify the source of the error in the log, we'll
also need our own exception class that identifies the source explicitly, such as
a fully qualified function name.

   }
   if (type != REG_SZ)
   {
     throw std::runtime_error("Value is not string type");

[sergei, 2014/07/28 11:46:27]

The same as above, it's completely not descriptive.

   }
 
   /*
    * Step two is to allocate a buffer for the string and query for its value.
    * Note that 'size' is the size in bytes, which we need for the system call,
    *   but that 'psize' is the size in words, which we need to manipulate the wchar_t array 'p'.
    */
+  size_t psize = (size + 1)/2;
   // Round the byte size up to the nearest multiple of two.
-  size = (size + 1) & ~ DWORD(1);
-  size_t psize = size >> 1;
+  size = 2 * psize;
   // We need to allocate a temporary buffer to receive the value, because there's no interface to write directly into the buffer of std::basic_string.
-  std::unique_ptr< wchar_t[] > p(new wchar_t[ psize ]);

[sergei, 2014/07/28 11:46:27]

not sure about it, One one hand it's correct, but I would like to declare
`std::wstring retValue(psize, L'\0');` here.

[Eric, 2014/07/29 12:42:25]

We need an allocated buffer, because writing into buffers declared const
violates class invariants.

[sergei, 2014/07/29 14:45:56]

But we have already decided, that `&x[0]` is OK for us.

[Eric, 2014/07/29 15:17:22]

I am not convinced that we have already decided this. I certainly have not
conceded the point.

+  std::unique_ptr<wchar_t[]> p(new wchar_t[psize]);
   hr = RegQueryValueExW(key, name.c_str(), 0, 0, reinterpret_cast<BYTE*>(p.get()), &size);
   if (hr != ERROR_SUCCESS)
   {
     throw std::runtime_error("Failure in RegQueryValueExW to retrieve value");
   }
 
   /*
    * Step three is to construct a return string.
    *
    * There's the possibility of an extra terminating null character in the return value of the query.
    * If so, we have to decrement the length of the return value to eliminate it.
    * If it persists, it will interfere with later string operations such as concatenation.
    */
-  if (p[ psize - 1 ] == L'\0')

[Oleksandr, 2014/07/27 22:13:30]

Nit: No spaces before and after array index: 
if (p[psize - 1] == L'\0')

+  if (p[psize - 1] == L'\0')
   {
     --psize;
   }
   return std::wstring(p.get(), psize);
 }