Issue #404 - Create common library shared between plugin/engine and installer

Issue #404 - Create common library shared between plugin/engine and installer

Create new static library 'common' as a separate project with its own
directory. It uses a separate .gypi include file so that it can be included in
both the plugin/engine solution and in the installer solution. This change set
does not incorporate changes to the installer, because it's not using these
modules yet.

Contents of the library are the registry and IE version classes. Split out the
unit tests for these two modules into separate source files. Refactor the unit
tests to conform to naming conventions. Add one unit test for Registry.

[Oleksandr, 2015-02-04 10:00:41]

* I am not convinced we need yet another shared project. Why can't we just use
the project "shared" in installer as well?
* I don't understand what this has to do to issue 404.
* The changes in test files don't seem to be related here at all
* The renaming of IE_Version.h is also unrelated, IMHO. Besides, if renaming it
I would rename it to IeVersion.h

http://codereview.adblockplus.org/6216090891845632/diff/5629499534213120/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5629499534213120/comm...
common/common.gypi:1: # This file is part of Adblock Plus
<http://adblockplus.org/>,
https://

[Eric, 2015-02-04 16:13:02]

On 2015/02/04 10:00:41, Oleksandr wrote:
> * I am not convinced we need yet another shared project. Why can't we just use
> the project "shared" in installer as well?

The scope is sharing is different. The current share is plugin/engine. This one
is plugin/engine/installer. There's plenty of code in "shared" that will always
be specific to only the plugin and engine; it's not a general-purpose library.

Whatever project we use, its sources will appear in the VS solution. I would
prefer, if possible, to leave extraneous code out.

> * I don't understand what this has to do to issue 404.

Issue 404 says that we should check the version of IE in the installer. The code
that currently does this in the plugin is not visible in the installer. This
change moves the code that performs that check into a new library "common". All
the other changes follow from this one.

> * The changes in test files don't seem to be related here at all

We seem to have been moving to splitting up test files into single units (or
small sets of related units). I simply took the opportunity to split the files
up at the same time they were being moved, combining two related changes.

> * The renaming of IE_Version.h is also unrelated, IMHO. Besides, if renaming
it
> I would rename it to IeVersion.h

OK. It's harder to read and it's not an identifier in code, but I don't care
that much.

[Oleksandr, 2015-02-04 21:45:31]

On 2015/02/04 16:13:02, Eric wrote:
> The scope is sharing is different. The current share is plugin/engine. This
one
> is plugin/engine/installer. There's plenty of code in "shared" that will
always
> be specific to only the plugin and engine; it's not a general-purpose library.

Still, I would just use the shared project in installer as well. This seems like
an over-engineering to me. Let's see what @sergei thinks.

[sergei, 2015-02-05 13:18:21]

I have not check the changes in details yet.

On 2015/02/04 21:45:31, Oleksandr wrote:
> On 2015/02/04 16:13:02, Eric wrote:
> > The scope is sharing is different. The current share is plugin/engine. This
> one
> > is plugin/engine/installer. There's plenty of code in "shared" that will
> always
> > be specific to only the plugin and engine; it's not a general-purpose
library.
> 
> Still, I would just use the shared project in installer as well. This seems
like
> an over-engineering to me. Let's see what @sergei thinks.

I would like to have a separate project.
Though, I also find it as over-engineering and the name is not so good. In ideal
case it would be better to have separate projects for communication, for
dictionary and utils project with the rest. But the proposed dividing is more
practical now.

What concerns the issue 404, I would like to have another issue for this
codereview because the current changes don't address #404, they are merely a
refactoring step which is in that direction. Issue 404 should depend on the new
issue.

[Eric, 2015-02-11 17:07:41]

On 2015/02/04 21:45:31, Oleksandr wrote:
> Still, I would just use the shared project in installer as well. This seems
like
> an over-engineering to me.

On 2015/02/05 13:18:21, sergei wrote:
> I would like to have a separate project.

So which is it? I'm blocked working on #404 until I can get this code accessible
to the installer.

You've both mentioned over-engineering, but I don't see what either of you are
concerned with. It's just another project, another container for code.

I'm blocked on even making new patch sets until we have a decision on this.


On 2015/02/05 13:18:21, sergei wrote:
> What concerns the issue 404, I would like to have another issue for this
> codereview because the current changes don't address #404, they are merely a
> refactoring step which is in that direction. Issue 404 should depend on the
new
> issue.

I would not need to make this changed but for #404; there's no reason for it
otherwise. To my mind that's a completely adequate reason for using the existing
issue. This change set is part of working on #404, not something independent of
it in any way

A separate issue would be more confusing, because it's not actually a separate
refactoring issue but only exists to serve one single other issue.

[Oleksandr, 2015-02-27 06:06:19]

It seems that we both of you agree that we need a separate project. I like the
current approach more than separate projects for dictionary, communication and
utils.

With that concept agreed upon the present codereview seems fine. Assuming
IE_Version -> IeVersion and the link in header is fixed.




On 2015/02/11 17:07:41, Eric wrote:
> On 2015/02/04 21:45:31, Oleksandr wrote:
> > Still, I would just use the shared project in installer as well. This seems
> like
> > an over-engineering to me.
> 
> On 2015/02/05 13:18:21, sergei wrote:
> > I would like to have a separate project.
> 
> So which is it? I'm blocked working on #404 until I can get this code
accessible
> to the installer.
> 
> You've both mentioned over-engineering, but I don't see what either of you are
> concerned with. It's just another project, another container for code.
> 
> I'm blocked on even making new patch sets until we have a decision on this.
> 
> 
> On 2015/02/05 13:18:21, sergei wrote:
> > What concerns the issue 404, I would like to have another issue for this
> > codereview because the current changes don't address #404, they are merely a
> > refactoring step which is in that direction. Issue 404 should depend on the
> new
> > issue.
> 
> I would not need to make this changed but for #404; there's no reason for it
> otherwise. To my mind that's a completely adequate reason for using the
existing
> issue. This change set is part of working on #404, not something independent
of
> it in any way
> 
> A separate issue would be more confusing, because it's not actually a separate
> refactoring issue but only exists to serve one single other issue.

[Eric, 2015-02-27 13:55:10]

Patch set two has the changes Oleksandr asked for.

http://codereview.adblockplus.org/6216090891845632/diff/5629499534213120/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5629499534213120/comm...
common/common.gypi:1: # This file is part of Adblock Plus
<http://adblockplus.org/>,
On 2015/02/04 10:00:41, Oleksandr wrote:
> https://

Done.

[Oleksandr, 2015-03-04 05:26:29]

Just 2 more comments

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/common.gypi:53: 'SubSystem': '1',   # Console
Nit: No space after #

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/include/IeVersion.h (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/include/IeVersion.h:25: namespace IE
Do we actually need these namespaces here? We don't generally have them in IE
code anyway. Can't we just drop these? Looks kind of C#-y this way, IMO.

[sergei, 2015-03-04 08:02:00]

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/common.gypi:53: 'SubSystem': '1',   # Console
On 2015/03/04 05:26:29, Oleksandr wrote:
> Nit: No space after #

Did you mean "one space before #"? Actually, as I see, we use space after #. But
we usually put comments in gyp files on a separate line before the commented
line.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/include/IeVersion.h (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/include/IeVersion.h:25: namespace IE
On 2015/03/04 05:26:29, Oleksandr wrote:
> Do we actually need these namespaces here? We don't generally have them in IE
> code anyway. Can't we just drop these? Looks kind of C#-y this way, IMO.

If we drop them then we need to rename the methods. I guess, `namespace IE` here
is equivalent of `class IE`, I personally like it more than static methods of
the class.

[Eric, 2015-03-04 14:15:16]

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/common.gypi:53: 'SubSystem': '1',   # Console
On 2015/03/04 05:26:29, Oleksandr wrote:
> Nit: No space after #

This 'msvs_settings' section is copypasta from the main GYP file
'adblockplus.gyp', where it appears twice.

Is there a need to change this, really?

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/include/IeVersion.h (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/include/IeVersion.h:25: namespace IE
On 2015/03/04 05:26:29, Oleksandr wrote:
> Do we actually need these namespaces here? We don't generally have them in IE
> code anyway.

First, I'd like to observe that the original purpose for this change set was to
alter the build. I split up one of the test files to conform to the split-file
pattern we're moving to and cleaned up some old names in the process. Renaming
things that aren't part of the build, though, is getting out of scope for the
present change set.

> Can't we just drop these? Looks kind of C#-y this way, IMO.

It looks like the pattern we're using in libadblockplus.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/include/IeVersion.h:25: namespace IE
On 2015/03/04 08:02:00, sergei wrote:
> If we drop them then we need to rename the methods. I guess, `namespace IE`
here
> is equivalent of `class IE`, I personally like it more than static methods of
> the class.

That's exactly why I used a namespace here in the first place. It's bad form to
use public static functions in a class when those function do not touch any
static data for this class. We have a whole lot of such functions in the code
already that ought to be converted to free-standing functions, whether in a
namespace or not. (Example: 'UnescapeUrl' used to be public static function.)

I used a namespace here because these version functions were always intended to
be used in more than just the plugin or engine.

[Oleksandr, 2015-03-04 15:37:23]

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/common.gypi:53: 'SubSystem': '1',   # Console
Yes, I actually was meant to say one space _before_ #, sorry. This can be fixed
in adblockplus.gyp sometime later as well.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/include/IeVersion.h (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/include/IeVersion.h:25: namespace IE
Why does it have to be a namespace or a class? Can't these 2 function be just
free floating as we have them in Utils.h or in PluginUtil.h?

We shouldn't be aiming at having the same namespace in libadblockplus and in IE.
IE code is different, so here it would have to be something like AdblockPlusIe.

I understand the namespaces weren't introduced in the patchset though, so yes,
it's a good idea to do it in a separate commit.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/test/IeVersionTest.cpp (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/test/IeVersionTest.cpp:25: #if !defined(DISABLE_EXACT_TESTS)
How about just using if !defined(INSTALLED_IE_VERSION) instead? Otherwise on
some systems we might have a test that will always fail. And there would be less
redundancy, IMO.

[sergei, 2015-03-04 15:53:55]

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/test/IeVersionTest.cpp (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/test/IeVersionTest.cpp:23: * If they are disabled, gtext will report 2
disabled tests.
Just to note, gtest, not gtext.

[Eric, 2015-03-04 16:55:10]

New patch set.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/common.gypi (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/common.gypi:53: 'SubSystem': '1',   # Console
On 2015/03/04 15:37:23, Oleksandr wrote:
> Yes, I actually was meant to say one space _before_ #, sorry. This can be
fixed
> in adblockplus.gyp sometime later as well. 

I fixed all of them. We're already altering adblockplus.gyp; no reason not to do
it now.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/include/IeVersion.h (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/include/IeVersion.h:25: namespace IE
On 2015/03/04 15:37:23, Oleksandr wrote:
> I understand the namespaces weren't introduced in the patchset though, so yes,
> it's a good idea to do it in a separate commit.

Yes. Good.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/test/IeVersionTest.cpp (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/test/IeVersionTest.cpp:23: * If they are disabled, gtext will report 2
disabled tests.
On 2015/03/04 15:53:56, sergei wrote:
> Just to note, gtest, not gtext.

Done.

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/test/IeVersionTest.cpp:25: #if !defined(DISABLE_EXACT_TESTS)
On 2015/03/04 15:37:23, Oleksandr wrote:
> How about just using if !defined(INSTALLED_IE_VERSION) instead? Otherwise on
> some systems we might have a test that will always fail. And there would be
less
> redundancy, IMO.

If we change it, we can change it elsewhere and later.

For the record, though, there are two definitions because they have two
different purposes.

1) INSTALLED_IE_VERSION enables the main repository can track whatever the
current release of IE is. This is independent of whether the current development
configuration is using it or not.

2) DISABLE_EXACT_TESTS is for quick suppression for testing with previous
versions, presumably in a VM, and temporarily so by mashing over the default
command line definitions in the project file generated by GYP. If you don't want
to suppress the test for whatever reason, define INSTALLED_IE_VERSION instead.

The only reason the test might _always_ fail is if there's no IE installed or
the current IE isn't installed. The first case is irrelevant and the second case
has not been problematic.

[Felix Dahlke, 2015-03-05 13:33:06]

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/test/IeVersionTest.cpp (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/test/IeVersionTest.cpp:25: #if !defined(DISABLE_EXACT_TESTS)
> If we change it, we can change it elsewhere and later.

The code is being introduced here - if we want to change it let's do it now. We
have enough questionable code we hope to change some day in IE as it is.

> The only reason the test might _always_ fail is if there's no IE installed or
the current IE isn't installed.

Not everyone is on IE11, stuff shouldn't fail by default on common systems. And
if we don't run the tests by default, DISABLE_EXACT_TESTS is entirely redundant
indeed.

[Eric, 2015-03-05 14:49:48]

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
File common/test/IeVersionTest.cpp (right):

http://codereview.adblockplus.org/6216090891845632/diff/5676830073815040/comm...
common/test/IeVersionTest.cpp:25: #if !defined(DISABLE_EXACT_TESTS)
On 2015/03/05 13:33:06, Felix H. Dahlke wrote:
> The code is being introduced here

It is not being introduced here. It's being moved from one directory to another.
Mercurial records that as a deletion and a new file, so it looks in the review
tool like brand new code.

[Eric, 2015-03-05 14:53:05]

Let's try that again.

On 2015/03/05 13:33:06, Felix H. Dahlke wrote:
> The code is being introduced here
 
On 2015/03/05 14:49:48, Eric wrote:
> It is not being introduced here. It's being moved from one directory to
another.
> Mercurial records that as a deletion and a new file, so it looks in the review
> tool like brand new code.

I misspoke. What's actually happening is that unit tests that were previously
combined in a single file is being split into two files.

What I was correct about the first time is that this is not new code. I had
slipped up on why it wasn't new code.

[Felix Dahlke, 2015-03-05 14:59:17]

On 2015/03/05 14:53:05, Eric wrote:
> Let's try that again.
> 
> On 2015/03/05 13:33:06, Felix H. Dahlke wrote:
> > The code is being introduced here
>  
> On 2015/03/05 14:49:48, Eric wrote:
> > It is not being introduced here. It's being moved from one directory to
> another.
> > Mercurial records that as a deletion and a new file, so it looks in the
review
> > tool like brand new code.
> 
> I misspoke. What's actually happening is that unit tests that were previously
> combined in a single file is being split into two files.
> 
> What I was correct about the first time is that this is not new code. I had
> slipped up on why it wasn't new code.

Oh, fair enough then! (Feel free to move me to CC again, just wanted to say my
two cents here.)

[Oleksandr, 2015-03-06 03:23:51]

LGTM

[sergei, 2015-04-02 07:46:18]

LGTM