Create a shared dictionary class for plugin and engine

This also moves the locales into the application install directory as well as switches to a better format (no separate files for the settings page strings, per-language dictionaries so unused languages don`t need to be read in and sections for better grouping of the strings).

[Eric, 2013-06-07 14:06:23]

Comment on installer issues.

http://codereview.adblockplus.org/10897028/diff/1/WixInstaller/adblockplusie.wxs
File WixInstaller/adblockplusie.wxs (left):

http://codereview.adblockplus.org/10897028/diff/1/WixInstaller/adblockplusie....
WixInstaller/adblockplusie.wxs:58: -->
These installer changes are LGTM.

Pretty much identical to what I would have written myself.

[Wladimir Palant, 2013-06-10 08:45:06]

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/Plugin.cpp
File src/plugin/Plugin.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/Plugin.cpp#newco...
src/plugin/Plugin.cpp:142: Dictionary::Create(locale);
I realized that this isn't the right place to initialize the dictionary,
https://hg.adblockplus.org/adblockplusie/rev/2beaca2e00b2 moved that code to the
right place.

[Felix Dahlke, 2013-06-11 12:31:14]

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/AdblockPlusClien...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:21: CString params = L"AdblockPlusEngine.exe "
+ CPluginSystem::GetInstance()->GetBrowserLanguage();
From the documentation
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms682429%28v=vs.85%29...),
it seems we could use engineExecutablePath in params (params =
engineExecutablePath + language), passing 0 for lpApplicationName.

If we really do need to pass the executable name in twice, I'd prefer having a
constant for it.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:20: if (locale != baseLocale &&
!ReadDictionary(basePath, locale))
It looks like if this or the other ReadDictionary call below fail, some of the
data in the default dictionary may already have been overwritten. I assume
that's intended?

If it's unintended, we could avoid this and one unnecessary ReadDictionary call
by first attempting to load the dictionary for the full locale, then for the
short locale and only if all that fails would we load the default locale.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:50: while (!stream.eof())
I think the following would suffice, it'd be a bit less code:

std::string line;
while (std::getline(stream, line))
{
...

I'm not really sure if we need to check for stream.fail(), but I guess it won't
hurt.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:63: else if (line.size() >= 1 && line[0] == '#')
This won't allow for comments preceeded by whitespace, or comments after text.

How about simply removing comments, like this?

std::string::size_type firstComment = line.find("#");
if (firstComment != std::string::npos)
  line = line.substring(0, firstComment);

[Wladimir Palant, 2013-06-12 13:46:07]

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/AdblockPlusClien...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:21: CString params = L"AdblockPlusEngine.exe "
+ CPluginSystem::GetInstance()->GetBrowserLanguage();
On 2013/06/11 12:31:14, Felix H. Dahlke wrote:
> From the documentation
>
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms682429%2528v=vs.85%...),
> it seems we could use engineExecutablePath in params (params =
> engineExecutablePath + language), passing 0 for lpApplicationName.

I would be opposed to that idea given that engineExecutablePath can contain
spaces and we will need to escape those then. It doesn't really matter what the
first parameter is which is why I don't pass in the full path here.

Agreed on adding a constant.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:20: if (locale != baseLocale &&
!ReadDictionary(basePath, locale))
On 2013/06/11 12:31:14, Felix H. Dahlke wrote:
> It looks like if this or the other ReadDictionary call below fail, some of the
> data in the default dictionary may already have been overwritten. I assume
> that's intended?

Yes, it is intended - even incomplete localization is better than no
localization. Also, ReadDictionary was intentionally written in such a way that
it doesn't fail (trash is ignored). It returns false only if the input file
really cannot be opened.

> If it's unintended, we could avoid this and one unnecessary ReadDictionary
call
> by first attempting to load the dictionary for the full locale, then for the
> short locale and only if all that fails would we load the default locale.

Given that the idea here is to deal with incomplete locales - that would be a
bad idea.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:50: while (!stream.eof())
On 2013/06/11 12:31:14, Felix H. Dahlke wrote:
> while (std::getline(stream, line))

IMHO that will get us into an endless loop - std::getline always returns the
stream reference. Normally the streams won't even throw exceptions on failure
which is why checking stream.eof() and stream.fail() is necessary from all I
know.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:63: else if (line.size() >= 1 && line[0] == '#')
We call TrimLine earlier so whitespace is allowed. As to comments that aren't at
the beginning of the line, the following code got our subscriptionlist
maintainers in trouble (parsed by Python's configparser):

  [Section #1]

So I better don't make comments all too flexible.

[Felix Dahlke, 2013-06-13 18:07:36]

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/AdblockPlusClien...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:21: CString params = L"AdblockPlusEngine.exe "
+ CPluginSystem::GetInstance()->GetBrowserLanguage();
On 2013/06/12 13:46:07, Wladimir Palant wrote:
> On 2013/06/11 12:31:14, Felix H. Dahlke wrote:
> > From the documentation
> >
>
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms682429%252528v=vs.8...),
> > it seems we could use engineExecutablePath in params (params =
> > engineExecutablePath + language), passing 0 for lpApplicationName.
> 
> I would be opposed to that idea given that engineExecutablePath can contain
> spaces and we will need to escape those then. It doesn't really matter what
the
> first parameter is which is why I don't pass in the full path here.
> 
> Agreed on adding a constant.

Fair enough.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:20: if (locale != baseLocale &&
!ReadDictionary(basePath, locale))
On 2013/06/12 13:46:07, Wladimir Palant wrote:
> On 2013/06/11 12:31:14, Felix H. Dahlke wrote:
> > It looks like if this or the other ReadDictionary call below fail, some of
the
> > data in the default dictionary may already have been overwritten. I assume
> > that's intended?
> 
> Yes, it is intended - even incomplete localization is better than no
> localization. Also, ReadDictionary was intentionally written in such a way
that
> it doesn't fail (trash is ignored). It returns false only if the input file
> really cannot be opened.
> 
> > If it's unintended, we could avoid this and one unnecessary ReadDictionary
> call
> > by first attempting to load the dictionary for the full locale, then for the
> > short locale and only if all that fails would we load the default locale.
> 
> Given that the idea here is to deal with incomplete locales - that would be a
> bad idea.

Okay, just wasn't sure if it was intended.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:50: while (!stream.eof())
On 2013/06/12 13:46:07, Wladimir Palant wrote:
> On 2013/06/11 12:31:14, Felix H. Dahlke wrote:
> > while (std::getline(stream, line))
> 
> IMHO that will get us into an endless loop - std::getline always returns the
> stream reference. Normally the streams won't even throw exceptions on failure
> which is why checking stream.eof() and stream.fail() is necessary from all I
> know.

No, it wouldn't be an endless loop, this is the canonical way to use
std::getline. The operator void*() method of the returned stream reference is
called, which returns 0 when reading failed. Like I said, not even sure if
stream.fail() is still necessary if we do this.

It's true that streams don't throw exceptions, that's what stream.fail() checks
for. Although you can enable exceptions for streams:
http://stackoverflow.com/a/3180285/606036

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:63: else if (line.size() >= 1 && line[0] == '#')
On 2013/06/12 13:46:07, Wladimir Palant wrote:
> We call TrimLine earlier so whitespace is allowed. As to comments that aren't
at
> the beginning of the line, the following code got our subscriptionlist
> maintainers in trouble (parsed by Python's configparser):
> 
>   [Section #1]
> 
> So I better don't make comments all too flexible.

Hm. One benefit of this is that the translated strings could contain #. It's
fine.

[Oleksandr, 2013-06-18 07:41:09]

LGTM in general. Just 2 comments.

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (left):

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1527: 
This is a menu that has been shown when a user clicks the Simple Adblock icon in
the status bar. We don't really use this menu anymore. It's been kept here in
case decide to present it again. Since that doesn't seem like to happen, I
suppose we can just remove it.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
I think it wouldn't hurt to use "filebuf::sh_read " just in case.

[Wladimir Palant, 2013-06-18 08:17:48]

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (left):

http://codereview.adblockplus.org/10897028/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1527: 
On 2013/06/18 07:41:09, Oleksandr wrote:
> This is a menu that has been shown when a user clicks the Simple Adblock icon
in
> the status bar. We don't really use this menu anymore. It's been kept here in
> case decide to present it again. Since that doesn't seem like to happen, I
> suppose we can just remove it.

It's quite the opposite actually - we should make that menu show up again. Not
with menu items like "invite friends" of course but at least disabling ABP on a
domain is must-have for the final release.

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
Good point even though this is non-standard, will do that.

[Wladimir Palant, 2013-11-05 11:30:25]

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
On 2013/06/18 07:41:09, Oleksandr wrote:
> I think it wouldn't hurt to use "filebuf::sh_read " just in case.

After looking into this I am less convinced that this is a good idea. There is
no std::filebuf::sh_read as this parameter isn't part of STL. filebuf::sh_read
on the other hand requires including fstream.h (*not* fstream) and seems to be
kept for reasons of backwards compatibility only. As I see it, we shouldn't be
using it. Did I miss anything?

[Eric, 2013-11-05 13:56:05]

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
On 2013/11/05 11:30:25, Wladimir Palant wrote:
>Did I miss anything?

The full constructor is declared thus:

    explicit ifstream (const char* filename, ios_base::openmode mode =
ios_base::in);
    explicit ifstream (const string& filename, ios_base::openmode mode =
ios_base::in);

"ios_base::in" is the portable version of "filebuf::sh_read", and it's there by
default. As I recall, the "filebuf" extensions predate C++98, though I'm hazy on
that (of course).

The upshot: Default constructor argument is just fine.

[Wladimir Palant, 2013-11-06 08:51:46]

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
On 2013/11/05 13:56:06, Eric wrote:
> "ios_base::in" is the portable version of "filebuf::sh_read", and it's there
by
> default.

Not really. If you look at
http://msdn.microsoft.com/en-us/library/aa277521%28v=vs.60%29.aspx, we are
talking about different parameters here - filebuf::sh_read is "protection mode"
(file locking flag). Still, I think that read sharing is what we get for an
input stream by default.

[Eric, 2013-11-06 17:49:53]

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
After realizing that I hadn't remembered what was happening correctly, I looked
into it.

On 2013/11/06 08:51:46, Wladimir Palant wrote:
> Still, I think that read sharing is what we get for an
> input stream by default.

Apparently not. See
http://stackoverflow.com/questions/27700/c-opening-a-file-in-non-exclusive-mode
for some exposition on it.

There's no C++ standard way of ensuring this; that much I'm fairly sure of.
There's a slim possibility of failure if, say, two plugin instances try to run
ReadDictionary at the same time. The easiest way of dealing with it is to use
Oleksandr's original suggestion. It's not portable, but this code is not
currently being ported, either.

The GNU C++ library has non-standard exceptions to the ifstream constructor as
well (see http://www.math.utah.edu/docs/info/iostream_4.html). If we ever need a
portable solution, we'd need to make this parametric in some way. The cleanest
C++ was is with templates, but that solution is premature, because there's no
actual problem yet, just a potential future one.

I think what I'd do is to add the non-standard mode with a big comment about
what the goal is, which is essentially something like a "ifstream_readonly"
class.

[Felix Dahlke, 2013-11-06 23:09:50]

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
On 2013/11/06 17:49:53, Eric wrote:
> After realizing that I hadn't remembered what was happening correctly, I
looked
> into it.
> 
> On 2013/11/06 08:51:46, Wladimir Palant wrote:
> > Still, I think that read sharing is what we get for an
> > input stream by default.
> 
> Apparently not. See
>
http://stackoverflow.com/questions/27700/c-opening-a-file-in-non-exclusive-mode
> for some exposition on it.
> 
> There's no C++ standard way of ensuring this; that much I'm fairly sure of.
> There's a slim possibility of failure if, say, two plugin instances try to run
> ReadDictionary at the same time. The easiest way of dealing with it is to use
> Oleksandr's original suggestion. It's not portable, but this code is not
> currently being ported, either.
> 
> The GNU C++ library has non-standard exceptions to the ifstream constructor as
> well (see http://www.math.utah.edu/docs/info/iostream_4.html). If we ever need
a
> portable solution, we'd need to make this parametric in some way. The cleanest
> C++ was is with templates, but that solution is premature, because there's no
> actual problem yet, just a potential future one.
> 
> I think what I'd do is to add the non-standard mode with a big comment about
> what the goal is, which is essentially something like a "ifstream_readonly"
> class.

This seems a bit like premature optimisation to me - even if we don't have read
sharing, are we sure it's going to be a problem? Can we test that this is not
the default behaviour? Otherwise I wouldn't bother adding filebuf::sh_read. We
can still look into that if this turns out to be a performance bottleneck later.

[Wladimir Palant, 2013-11-07 06:53:57]

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp
File src/shared/Dictionary.cpp (right):

http://codereview.adblockplus.org/10897028/diff/1/src/shared/Dictionary.cpp#n...
src/shared/Dictionary.cpp:45: std::ifstream stream(basePath + locale + L".ini");
According to http://msdn.microsoft.com/en-us/library/vstudio/zek0beca.aspx
std::ifstream constructor also has a Microsoft-specific _Prot parameter
defaulting to ios_base::_Openprot. Looking inside xiosbase header where it is
defined I see the following:

#define _OPENPROT     _SH_DENYNO

Apparently, we could simply pass in some sharing flag from Share.h here.
However, the default is already the most permissive sharing mode which is
exactly what we need (the dictionary files are only being modified by the
installer and the application isn't running there). I guess I can close that
review then, we don't have to do anything.