Issue 1283 - wrong usage of memset, fix sizeof, make proper initializing

[Eric, 2014-10-08 16:59:14]

Sergei, the entire DIFF seems like it's got original and revision reversed. It
probably needs a new patch set. I refrained from making comments so that you can
simply delete the erroneous patch set without guilt.

[sergei, 2014-10-08 21:38:46]

On 2014/10/08 16:59:14, Eric wrote:
> Sergei, the entire DIFF seems like it's got original and revision reversed. It
> probably needs a new patch set. I refrained from making comments so that you
can
> simply delete the erroneous patch set without guilt.

Thanks a lot, sorry, the patch is re-uploaded.

[Eric, 2014-10-09 13:49:57]

Almost all good.

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
File src/plugin/PluginSystem.cpp (right):

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 9> localeLanguage;
I think that page means that you have nine characters for the locale and one
terminating null. I say I think because the wording isn't totally clear. I'd use
an array length of 10 to be safe.

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:14: localeLanguage.fill(L'\0');
We can initialize with localLanguage[0] = '\0' as elsewhere here.

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:28: localeCountry.fill(L'\0');
Also here.

[sergei, 2014-10-13 09:48:11]

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
File src/plugin/PluginSystem.cpp (right):

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 9> localeLanguage;
On 2014/10/09 13:49:57, Eric wrote:
> I think that page means that you have nine characters for the locale and one
> terminating null. I say I think because the wording isn't totally clear. I'd
use
> an array length of 10 to be safe.
It's quite clear, the terminating character is included in these nine characters
!> The maximum number of characters allowed for this string is nine, including a
terminating null character.

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:14: localeLanguage.fill(L'\0');
On 2014/10/09 13:49:57, Eric wrote:
> We can initialize with localLanguage[0] = '\0' as elsewhere here. 
Since we check the result code of GetLocaleInfo call, there is no need to set
any character to null. So I've removed it.

[Eric, 2014-10-14 18:33:36]

LGTM

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
File src/plugin/PluginSystem.cpp (right):

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 9> localeLanguage;
On 2014/10/13 09:48:11, sergei wrote:
> It's quite clear, the terminating character is included in these nine
characters

I'd still change it to 10, but it shouldn't matter, since we should only see at
most three characters for a language code. Frankly, I don't know why they think
they need even 9 characters.

http://codereview.adblockplus.org/5163581322559488/diff/5685265389584384/src/...
src/plugin/PluginSystem.cpp:14: localeLanguage.fill(L'\0');
On 2014/10/13 09:48:11, sergei wrote:
> Since we check the result code of GetLocaleInfo call

Works for me.

[Oleksandr, 2014-10-17 07:09:07]

LGTM. The comment I have is optional, really. I'm fine with committing this as
is.

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

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
src/plugin/PluginClass.cpp:905: int classNameLength = GetClassNameW(hTabWnd,
className.data(), className.size());
In other places of the code we use std::wstring in situations like this.
Like:

std::wstring className;
className.resize(MAX_PATH);
GetClassName(..., &className[0]...);

I agree, it might be less pretty, but if we do the same here we could use the
std::wstring's operator "==" instead of wcscmp, and we would be more consistent
across the code, which would be preferable, IMHO. And it would add some
consistence as to how we treat these cases.

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
File src/plugin/PluginSystem.cpp (right):

http://codereview.adblockplus.org/5163581322559488/diff/5700735861784576/src/...
src/plugin/PluginSystem.cpp:14: int res = GetLocaleInfoW(lcid,
LOCALE_SISO639LANGNAME, localeLanguage.data(), localeLanguage.size());
We could use std::wstring for localeLanguage here as well