Issue #1186 - Change names that appear in the custom action

Issue #1186 - Change names that appear in the custom action

The scope of this change set is to rename constant, variable, function, and 
class names that appear in the custom action code. These identifiers include
both those present only in the custom action code itself, as well as those
found in the installer library and its unit tests. The name of the entry point
to the custom action library is also present, which appears also in .DEF and
.WXS files.

The scope of this change set does not include identifier that only appear in
the installer, its unit tests, or elsewhere. Nor does it include identifiers
that are not constant, variable, function, or class names. These will be the
subject of future change sets.

Also, there's a removal of some dead code.

This change set blocks future change sets for #1186, given how many lines
any such future change set would have in common with the present one.

[Oleksandr, 2014-10-06 20:44:12]

*For anyone planning to review this - it's only about changing names to
camelCase.

I personally would prefer just changing all the names throughout all the
installer. Otherwise it's pretty much a naming mess. But if you can handle the
renaming of the rest soon enough, this LGTM.

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
File installer/src/installer-lib/session.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/installer-lib/session.cpp:30: log_prefix( name + ": " )
logPrefix?

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/installer-lib/session.cpp:49: struct Log_Message
LogMessage here?

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/installer-lib/session.cpp:73: write_message_noexcept( Log_Message(
log_prefix + message ) ) ;
WriteMessageNoexcept

[Eric, 2014-10-14 17:42:04]

On 2014/10/06 20:44:12, Oleksandr wrote:
> *For anyone planning to review this - it's only about changing names to
> camelCase.

Yes, and not even all the names. This round is only the names that appear
somewhere in the 'instaler-ca' project. The three names you mentioned in
comments only appear in the library implementation, so they were out of scope.
(To be fair, I changed all the enum names at the same time, because that just
looked awful, even temporarily.)

> I personally would prefer just changing all the names throughout all the
> installer. Otherwise it's pretty much a naming mess. But if you can handle the
> renaming of the rest soon enough, this LGTM.

That is exactly the plan. I split this up in order to limit the total number of
changed lines in the change set.

[Eric, 2014-10-14 17:42:49]

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
File installer/src/installer-lib/session.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/installer-lib/session.cpp:30: log_prefix( name + ": " )
On 2014/10/06 20:44:12, Oleksandr wrote:
> logPrefix?
Next round, as also with the two below. See review comment for more.

[Eric, 2014-10-14 17:45:01]

Sergei:

I just added you as reviewer and took Felix off the list. Last week when I
posted this he was here and you weren't. He didn't get to it in time.

Now the situation is reversed, so I've put you on.

[Felix Dahlke, 2014-10-15 02:46:57]

I'm here, actually.

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
File installer/src/custom-action/close_application.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:39: bool isRunning()
Functions should start with an upper case letter.

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
File installer/src/installer-lib/interaction.h (right):

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/installer-lib/interaction.h:33: defaultBox = 0,
Enum values should be all uppercase and with underscores, and also include the
enum name, e.g.:

enum BoxType
{
  BOX_TYPE_DEFAULT_BOX,
  BOX_TYPE_ERROR_BOX,
  ...
}

(I'll also point out that there's no need to use a typedef in C++, we never do
it - but that's a separate issue.)

[Eric, 2014-10-15 15:30:43]

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
File installer/src/custom-action/close_application.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:39: bool isRunning()
On 2014/10/15 02:46:57, Felix H. Dahlke wrote:
> Functions should start with an upper case letter.

Missed that one, apparently.

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
File installer/src/installer-lib/interaction.h (right):

http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
installer/src/installer-lib/interaction.h:33: defaultBox = 0,
On 2014/10/15 02:46:57, Felix H. Dahlke wrote:
> Enum values should be all uppercase and with underscores, and also include the
> enum name

I will first point out that our coding style documents are silent about enum
values, both https://adblockplus.org/en/coding-style and
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style .

All caps with underscores is the naming convention for preprocessor variables.
It would be better to keep such names referring only to these variables, simply
because there's less and less reason to use the preprocessor as the language
progresses.

There is, however, a hint in the Mozilla coding style about the right kind of
name. There's a name prefix 'k' listed for enumeration values for JavaScript,
but these names are otherwise CamelCase. I would rather use CamelCase for
enumerations than make them look like preprocessor variables.

> (I'll also point out that there's no need to use a typedef in C++, we never do
> it - but that's a separate issue.)

I'm changing this to 'enum class', since I now know that we've got this C++11
feature in the compiler. Because 'enum class' is properly scoped, it means that
there's no longer a reason to use a prefix the enumerations, since instead the
scoping operator '::' is used.

In any case, take a look at how these enumeration constants appear in the calls.
It's better than before, and much more readable that a preprocessor-style name
would be.

[Felix Dahlke, 2014-10-17 09:34:43]

Looks like you missed a few functions. Note that I only really looked over the
ones you did change - I'm not sure if you left the single word functions I've
noticed alone because you'll get to it later, or because you forgot to upper
case it.

On 2014/10/15 15:30:43, Eric wrote:
>
http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
> File installer/src/custom-action/close_application.cpp (right):
> 
>
http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
> installer/src/custom-action/close_application.cpp:39: bool isRunning()
> On 2014/10/15 02:46:57, Felix H. Dahlke wrote:
> > Functions should start with an upper case letter.
> 
> Missed that one, apparently.
> 
>
http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
> File installer/src/installer-lib/interaction.h (right):
> 
>
http://codereview.adblockplus.org/5219280069066752/diff/5629499534213120/inst...
> installer/src/installer-lib/interaction.h:33: defaultBox = 0,
> On 2014/10/15 02:46:57, Felix H. Dahlke wrote:
> > Enum values should be all uppercase and with underscores, and also include
the
> > enum name
> 
> I will first point out that our coding style documents are silent about enum
> values, both https://adblockplus.org/en/coding-style and
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
.
> 
> All caps with underscores is the naming convention for preprocessor variables.
> It would be better to keep such names referring only to these variables,
simply
> because there's less and less reason to use the preprocessor as the language
> progresses.
> 
> There is, however, a hint in the Mozilla coding style about the right kind of
> name. There's a name prefix 'k' listed for enumeration values for JavaScript,
> but these names are otherwise CamelCase. I would rather use CamelCase for
> enumerations than make them look like preprocessor variables.
> 
> > (I'll also point out that there's no need to use a typedef in C++, we never
do
> > it - but that's a separate issue.)
> 
> I'm changing this to 'enum class', since I now know that we've got this C++11
> feature in the compiler. Because 'enum class' is properly scoped, it means
that
> there's no longer a reason to use a prefix the enumerations, since instead the
> scoping operator '::' is used.
> 
> In any case, take a look at how these enumeration constants appear in the
calls.
> It's better than before, and much more readable that a preprocessor-style name
> would be.

Yeah, you're right. It's inconsistent in the Mozilla code base, and also in ABP
for IE, but I agree that the convention you suggest makes more sense. Fully
agree with using enum classes, too. I'll add a bit about this to the coding
style to clarify.

[Eric, 2014-10-20 01:45:05]

On 2014/10/17 09:34:43, Felix H. Dahlke wrote:
> Looks like you missed a few functions. Note that I only really looked over the
> ones you did change - I'm not sure if you left the single word functions I've
> noticed alone because you'll get to it later, or because you forgot to upper
> case it.

There are lots of functions I did not convert, but only because I declared them
outside the scope of this batch of renaming. If the function, variable, constant
was defined or used in a C++ source file that sits in
'installer/src/custom_action', I renamed it. If it was defined or used only in
the library code, I left it alone. If it was another kind of identifier, I left
it alone.

I'm planning on renaming everything, but I'm splitting this task into pieces to
make the reviews go smoother. Even this review is on the large side.

I did a double check by brute-force with 'grep -n _' and found a few more things
that qualify for the scope of this review, so there's another patch set.

[Felix Dahlke, 2014-10-22 16:00:32]

Not sure I fully got it :) But I've pointed out some stuff that should probably
be renamed in this batch according to this logic.

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
File installer/src/custom-action/close_application.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/custom-action/close_application.cpp:44: bool shutDown()
Should be ShutDown(), hm?

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
File installer/src/installer-lib/DLL.h (right):

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/installer-lib/DLL.h:24: static DllModule & module();
Should be Module, hm?

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
File installer/src/installer-lib/custom-i18n.h (right):

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/installer-lib/custom-i18n.h:30: std::wstring text( const
std::wstring id )
Should be Text(), hm?

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
File installer/src/installer-lib/process.h (right):

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/installer-lib/process.h:689: bool shutDown() ;
Should be ShutDown(), hm?

[Eric, 2015-01-04 19:02:43]

I think I've got them all. I think.

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
File installer/src/custom-action/close_application.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/custom-action/close_application.cpp:32: void refresh()
Caught this one.

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/custom-action/close_application.cpp:44: bool shutDown()
On 2014/10/22 16:00:33, Felix H. Dahlke wrote:
> Should be ShutDown(), hm?

Changed all of them, including two calls below and their definitions.

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/custom-action/close_application.cpp:138: session.log( "IE not
running. No issue with reboot policy." ) ;
Also this one.

http://codereview.adblockplus.org/5219280069066752/diff/5700735861784576/inst...
installer/src/custom-action/close_application.cpp:269:
messageText.text(L"dialog_unknown"),
And this one.

[Oleksandr, 2015-01-28 10:33:16]

LGTM yet again.

[Felix Dahlke, 2015-01-30 14:48:35]

LGTM. Don't really have the energy to fully dive into this and double check if
nothing was missed, but I guess we can fix the remains as we notice them.

[Felix Dahlke, 2015-01-30 14:53:24]

http://codereview.adblockplus.org/5219280069066752/diff/5752142325350400/inst...
File installer/src/installer-lib/interaction.cpp (right):

http://codereview.adblockplus.org/5219280069066752/diff/5752142325350400/inst...
installer/src/installer-lib/interaction.cpp:18: : Message(message,
INSTALLMESSAGE(long(box) | long(buttonSet) | long(defaultButton) | long(icon)))
I've assumed this is not going to be committed along with the rename BTW. We
have a rule against using C-style casts:
https://adblockplus.org/en/coding-style#cpp

But assuming it's not actually introduced along with the renames, we can fix it
later.

[Eric, 2015-02-02 12:59:25]

On 2015/01/30 14:53:24, Felix H. Dahlke wrote:
>
http://codereview.adblockplus.org/5219280069066752/diff/5752142325350400/inst...
> File installer/src/installer-lib/interaction.cpp (right):
> 
>
http://codereview.adblockplus.org/5219280069066752/diff/5752142325350400/inst...
> installer/src/installer-lib/interaction.cpp:18: : Message(message,
> INSTALLMESSAGE(long(box) | long(buttonSet) | long(defaultButton) |
long(icon)))
> I've assumed this is not going to be committed along with the rename BTW. We
> have a rule against using C-style casts:
> https://adblockplus.org/en/coding-style#cpp

We do have such a rule, and those are not C-style casts. They're C++ conversion
constructors. They do look similar at first glance. A cast would be
"(long)buttonSet"; same number of parentheses.

[Felix Dahlke, 2015-02-02 13:14:03]

On 2015/02/02 12:59:25, Eric wrote:
> On 2015/01/30 14:53:24, Felix H. Dahlke wrote:
> >
>
http://codereview.adblockplus.org/5219280069066752/diff/5752142325350400/inst...
> > File installer/src/installer-lib/interaction.cpp (right):
> > 
> >
>
http://codereview.adblockplus.org/5219280069066752/diff/5752142325350400/inst...
> > installer/src/installer-lib/interaction.cpp:18: : Message(message,
> > INSTALLMESSAGE(long(box) | long(buttonSet) | long(defaultButton) |
> long(icon)))
> > I've assumed this is not going to be committed along with the rename BTW. We
> > have a rule against using C-style casts:
> > https://adblockplus.org/en/coding-style#cpp
> 
> We do have such a rule, and those are not C-style casts. They're C++
conversion
> constructors. They do look similar at first glance. A cast would be
> "(long)buttonSet"; same number of parentheses.

I'm pretty sure that's not the correct term for scalar types (which don't have a
constructor), but yeah, that syntax is indeed not possible in C. Much more
importantly though: It's just different syntax for a C-style cast, it's got the
exact same semantics. And those we don't want, whatever it looks like.

[Eric, 2015-02-02 19:03:58]

On 2015/02/02 13:14:03, Felix H. Dahlke wrote:
> Much more
> importantly though: It's just different syntax for a C-style cast, it's got
the
> exact same semantics. And those we don't want, whatever it looks like.

I had to look up the details. When T is built-in type, then T(E) is the same as
(T)E. When T is not built-in, it's not. (I don't think I knew that rule so
explicitly before).

As I recall, those conversions are only there to suppress a spurious
type-mismatch warning from the compiler.