NoIssue - Refactor HTTP and HTTPS namespaces registration

Refactor HTTP and HTTPS namespaces registration in 2 pach sets. Second patchset renames CPluginMimeFilterClient to CAppNamespaceClient

[Oleksandr, 2015-04-13 03:34:51]

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

http://codereview.adblockplus.org/5032147387678720/diff/5668600916475904/src/...
src/plugin/PluginClass.cpp:251: s_mimeFilter.reset(new
CPluginAppNamespaceClient());
It doesn't look like CPluginAppNammespaceClient() actually throws anything, so
no need for being extra cautious here.

[sergei, 2015-04-22 11:39:40]

before reviewing would like to note that it seems renaming is missed in the
patch

[Eric, 2015-05-16 21:00:02]

The overall idea here is sound. Having a separate function that does mostly the
same thing as a destructor is just wrong, and that's going away; good.

I'm having some trouble with the review tool, though. I can't see all the
differences in one place, which make reviewing a bit more difficult. Could you
upload the entirety of the change set as a single whole?

Digging into the refactored code, I've seen a number of other defects. If we're
going to refactor, we should likely do a complete job of it.

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

http://codereview.adblockplus.org/5032147387678720/diff/5668600916475904/src/...
src/plugin/PluginClass.cpp:251: s_mimeFilter.reset(new
CPluginAppNamespaceClient());
On 2015/04/13 03:34:51, Oleksandr wrote:
> It doesn't look like CPluginAppNammespaceClient() actually throws anything, so
> no need for being extra cautious here.

It can throw. This constructor calls 'CMetaFactory::GetInstance', which calls
various ATL functions that can throw. But then again, it's never a good idea to
assume that something can't throw unless it's explicitly declared 'noexcept'.

It's not immediately apparent that this constructor can throw because it's
written to suppress errors. In order to make incremental progress, we can still
suppress errors, but we should be suppressing them by catching a constructor
exception.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/adbl...
File adblockplus.gyp (right):

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/adbl...
adblockplus.gyp:115: 'src/plugin/PluginAppNamespaceClient.h',
I'd recommend the root name "ProtocolHandler" instead of "AppNamespaceClient".
It says what the code does rather than how it's hooked into IE. If there were
some other way of registering the handler, "ProtocolHandler" would still be
accurate but "AppNamespace" would not be.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
File src/plugin/PluginAppNamespaceClient.cpp (right):

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginAppNamespaceClient.cpp:25: typedef
PassthroughAPP::CMetaFactory<PassthroughAPP::CComClassFactoryProtocol,WBPassthru>
MetaFactory;
This definition could go in an anonymous namespace.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginAppNamespaceClient.cpp:33: CComPtr<IInternetSession> spSession;
We're also calling 'CoInternetGetSession' in the destructor, and we're using it
to unregister what we're registering here. It seems that the session ought to be
a member variable.

Also 'spSession' is reminiscent of Hungarian notation, which is in contradiction
to style policy. Might as well take the opportunity to rename it.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginAppNamespaceClient.cpp:41: hr =
MetaFactory::CreateInstance(CLSID_HttpProtocol, &m_spCFHTTP);
The invocation of 'CreateInstance' starts a memory leak. The pointer assigned to
'm_spCFHTTP' is allocated with 'new' (I read the code in "ProtocolCF.inl") but
not deallocated in the destructor. That's because 'CreateInstance' calls 'new'
internally; the caller of 'CreateInstance' has to call 'delete'.

The fact that the declaration of 'm_spCFHTTP' is 'CComPtr' does not affect this.
The destructor of 'CComPtr' calls 'Release', but that doesn't cause the object
to be deallocated.

This is arguably a design defect (or at least a documentation deficiency) in
PassthroughAPP. Nevertheless, its documentation does say that it "returns a
non-AddRef'ed pointer", so we should not naively assume that COM allocation is
being used here.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginAppNamespaceClient.cpp:59: return;
If the "http" handler registers correctly, but this one doesn't, we've left this
object in an inconsistent state.

The point here is this constructor masks errors. For the interim, we will still
be masking errors one way or another. We can make progress by masking the errors
when the constructor is called, rather than masking them in the constructor and
letting the object be constructed in an invalid state.

The upshot is that many of the 'return' statements here ought to throw instead.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginAppNamespaceClient.cpp:78: {
We are manually unregistering both name spaces with identical code, just as we
are registering them above with identical code.

It would be better design to make each of these namespace an instance of a class
that manages the registration. Registration is a resource, and we're using RAII
when appropriate.

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
File src/plugin/PluginAppNamespaceClient.h (right):

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginAppNamespaceClient.h:22: class CPluginAppNamespaceClient
New classes we're writing are not prefixed with "CPlugin". I've never liked this
prefix; it's like a third-rate namespace.

Can we take the opportunity to drop it?

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

http://codereview.adblockplus.org/5032147387678720/diff/5733935958982656/src/...
src/plugin/PluginClass.cpp:251: s_mimeFilter.reset(new
CPluginAppNamespaceClient());
's_mimeFilter' should probably also be renamed in this change set.