Updated installer with custom action

Updated installer with custom action

[Oleksandr, 2014-03-12 19:26:03]

LGTM in general. Minor remarks and comments.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
File installer/installer.gyp (right):

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/installer.gyp:726: 'src/installer-lib/session.h',
We don't usually include .h files in GYP, I believe

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
File installer/src/custom-action/abp_ca.def (right):

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/abp_ca.def:3: ;
-------------------------------------------------------------------------------------------------
Where is it? :) And why only here?

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:16: class IE_Closer
I am not a fan of a name IE_Closer here. Seeing this also closes the engine,
wouldn't it be better to have something like ABP_Closer here instead?

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:18: Snapshot snapshot ;
Cosmetic note here and further. We don't usually have a space before the
semi-column. Like "Snapshot snapshot;"

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:26: //Process_Closer iec(
snapshot, IE_names, 2 ) ;
This should probably be deleted?

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:30: : snapshot(), ie_closer(
snapshot, ie_names, 1 ), engine_closer( snapshot, engine_names, 1 )
One more cosmetic comment - we don't usually have a space before or after the
bracket. Like:
" : snapshot(), ie_closer(snapshot, ie_names, 1), engine_closer(snapshot,
engine_names, 1)"

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:135: * We take the short path
through this function if IE is not running at the outset.
IE >or Engine<, actually

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:163: Policy_State state =
not_known;
Redundant initialization here

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:61: int wcscmpi( const wchar_t * s1,
const wchar_t * s2 )
Why don't we just use _wcsicmp instead?
http://msdn.microsoft.com/en-us/library/e0z9k731.aspx

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:94: int wcsncmpi( const wchar_t * s1,
const wchar_t * s2, unsigned int n )
_wcsnicmp? http://msdn.microsoft.com/en-us/library/chd90w8e.aspx

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/record.cpp:72: if ( x == 0xFFFFFFFF )
We use INVALID_HANDLE_VALUE pretty much everywhere else

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/session.cpp:10: #include <sstream>
Not needed anymore, is it?

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
File installer/src/msi/adblockplusie.wxs (right):

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:302: <?include bho_registry_value.wxi ?>
This isn't directly related to this codereview, but just so that we don't
forget. We also need to write an Implemented Categories registry key. Please see
src\Plugin\AdblockPlus.rgs for more info

[Eric, 2014-03-17 12:26:46]

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
File installer/installer.gyp (right):

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/installer.gyp:726: 'src/installer-lib/session.h',
On 2014/03/12 19:26:04, Oleksandr wrote:
> We don't usually include .h files in GYP, I believe
They're not necessary for the build, true. But also they don't interfere with
it.

I put them there so I could locate them faster when working inside Visual
Studio.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
File installer/src/custom-action/abp_ca.def (right):

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/abp_ca.def:3: ;
-------------------------------------------------------------------------------------------------
On 2014/03/12 19:26:04, Oleksandr wrote:
> Where is it? :) And why only here?
Acknowledged. I need to get copyright notices on all the code. Do we have a
policy for how the notices should appear? I looked for such a thing a while ago
and didn't fine one.

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:16: class IE_Closer
On 2014/03/12 19:26:04, Oleksandr wrote:
> Seeing this also closes the engine,
> wouldn't it be better to have something like ABP_Closer here instead?
Well, a proper name would indicate that we're closing all the assets that need
to be overwritten and all the programs that might hold them open and keep them
from closing. That's kind of a mouthful. I just picked the name that's the
ordinary target of the operation.

I think ABP_Closer is less descriptive, though I'm not opposed to a better name.
Such a name, though, should likely also affect "abp_close_ie" (the entry point
to the CA) and "Close_IE" (the name for the CA in WiX).

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:18: Snapshot snapshot ;
On 2014/03/12 19:26:04, Oleksandr wrote:
> Cosmetic note here and further. We don't usually have a space before the
> semi-column. Like "Snapshot snapshot;"
I seem to always use more whitespace than most. I've found over the years it
helps me read code faster. If we want to run everything through a consistent
code formatter, I won't object, though.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:26: //Process_Closer iec(
snapshot, IE_names, 2 ) ;
On 2014/03/12 19:26:04, Oleksandr wrote:
> This should probably be deleted?
Yep. That's the previous algorithm I was using to close.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:135: * We take the short path
through this function if IE is not running at the outset.
On 2014/03/12 19:26:04, Oleksandr wrote:
> IE >or Engine<, actually

Done.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/custom-action/close_application.cpp:163: Policy_State state =
not_known;
On 2014/03/12 19:26:04, Oleksandr wrote:
> Redundant initialization here

Done.

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:61: int wcscmpi( const wchar_t * s1,
const wchar_t * s2 )
On 2014/03/12 19:26:04, Oleksandr wrote:
> Why don't we just use _wcsicmp instead?
> http://msdn.microsoft.com/en-us/library/e0z9k731.aspx
We could use an MS-specific library function, I suppose. I have, though, an
automatic reflex to avoid MS-specific functions that are not platform functions.

Having looked at the documentation, it's not clear that the comparisons are
length-limited, which is a practice to be avoided for C-style strings (which is
how the process names are encoded in the API).

I'd rather avoid the C-style comparisons entirely, frankly. The better way of
dealing with though, would be to declare a case-insensitive wstring by using
traits. That was all the comparisons for process names would automatically be
case-insensitive.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/process.cpp:65: const size_t LIMIT( 4294967295 ) ;
// Runaway limit of 2^32 - 1 should be acceptably long.
This limit ought to be 2^16 - 1. I don't know why I set it so high.

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/record.cpp:72: if ( x == 0xFFFFFFFF )
On 2014/03/12 19:26:04, Oleksandr wrote:
> We use INVALID_HANDLE_VALUE pretty much everywhere else
This is the return value as documented on the MsiRecordGetFieldCount page. On
the other hand, that page also documents -1 as a possible return value for an
unsigned int. But whatever it is, it's not a handle value, nor an error-handle.

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

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/installer-lib/session.cpp:10: #include <sstream>
On 2014/03/12 19:26:04, Oleksandr wrote:
> Not needed anymore, is it?

It's used in Session::write_message.

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
File installer/src/msi/adblockplusie.wxs (right):

http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:302: <?include bho_registry_value.wxi ?>
On 2014/03/12 19:26:04, Oleksandr wrote:
> This isn't directly related to this codereview, but just so that we don't
> forget. We also need to write an Implemented Categories registry key. Please
see
> src\Plugin\AdblockPlus.rgs for more info 

This is for Windows 8?

[Oleksandr, 2014-07-15 14:19:13]

On 2014/03/17 12:26:46, Eric wrote:
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/installer.gyp (right):
> 
This can be closed, since we have Whole Installer review now.
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/installer.gyp:726: 'src/installer-lib/session.h',
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > We don't usually include .h files in GYP, I believe
> They're not necessary for the build, true. But also they don't interfere with
> it.
> 
> I put them there so I could locate them faster when working inside Visual
> Studio.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/src/custom-action/abp_ca.def (right):
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/custom-action/abp_ca.def:3: ;
>
-------------------------------------------------------------------------------------------------
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > Where is it? :) And why only here?
> Acknowledged. I need to get copyright notices on all the code. Do we have a
> policy for how the notices should appear? I looked for such a thing a while
ago
> and didn't fine one.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/src/custom-action/close_application.cpp (right):
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/custom-action/close_application.cpp:16: class IE_Closer
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > Seeing this also closes the engine,
> > wouldn't it be better to have something like ABP_Closer here instead?
> Well, a proper name would indicate that we're closing all the assets that need
> to be overwritten and all the programs that might hold them open and keep them
> from closing. That's kind of a mouthful. I just picked the name that's the
> ordinary target of the operation.
> 
> I think ABP_Closer is less descriptive, though I'm not opposed to a better
name.
> Such a name, though, should likely also affect "abp_close_ie" (the entry point
> to the CA) and "Close_IE" (the name for the CA in WiX).
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/custom-action/close_application.cpp:18: Snapshot snapshot ;
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > Cosmetic note here and further. We don't usually have a space before the
> > semi-column. Like "Snapshot snapshot;"
> I seem to always use more whitespace than most. I've found over the years it
> helps me read code faster. If we want to run everything through a consistent
> code formatter, I won't object, though.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/custom-action/close_application.cpp:26: //Process_Closer iec(
> snapshot, IE_names, 2 ) ;
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > This should probably be deleted?
> Yep. That's the previous algorithm I was using to close.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/custom-action/close_application.cpp:135: * We take the short
path
> through this function if IE is not running at the outset.
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > IE >or Engine<, actually
> 
> Done.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/custom-action/close_application.cpp:163: Policy_State state =
> not_known;
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > Redundant initialization here
> 
> Done.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/src/installer-lib/process.cpp (right):
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/installer-lib/process.cpp:61: int wcscmpi( const wchar_t * s1,
> const wchar_t * s2 )
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > Why don't we just use _wcsicmp instead?
> > http://msdn.microsoft.com/en-us/library/e0z9k731.aspx
> We could use an MS-specific library function, I suppose. I have, though, an
> automatic reflex to avoid MS-specific functions that are not platform
functions.
> 
> Having looked at the documentation, it's not clear that the comparisons are
> length-limited, which is a practice to be avoided for C-style strings (which
is
> how the process names are encoded in the API).
> 
> I'd rather avoid the C-style comparisons entirely, frankly. The better way of
> dealing with though, would be to declare a case-insensitive wstring by using
> traits. That was all the comparisons for process names would automatically be
> case-insensitive.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/installer-lib/process.cpp:65: const size_t LIMIT( 4294967295 ) ;
> // Runaway limit of 2^32 - 1 should be acceptably long.
> This limit ought to be 2^16 - 1. I don't know why I set it so high.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/src/installer-lib/record.cpp (right):
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/installer-lib/record.cpp:72: if ( x == 0xFFFFFFFF )
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > We use INVALID_HANDLE_VALUE pretty much everywhere else
> This is the return value as documented on the MsiRecordGetFieldCount page. On
> the other hand, that page also documents -1 as a possible return value for an
> unsigned int. But whatever it is, it's not a handle value, nor an
error-handle.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/src/installer-lib/session.cpp (right):
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/installer-lib/session.cpp:10: #include <sstream>
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > Not needed anymore, is it?
> 
> It's used in Session::write_message.
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> File installer/src/msi/adblockplusie.wxs (right):
> 
>
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...
> installer/src/msi/adblockplusie.wxs:302: <?include bho_registry_value.wxi ?>
> On 2014/03/12 19:26:04, Oleksandr wrote:
> > This isn't directly related to this codereview, but just so that we don't
> > forget. We also need to write an Implemented Categories registry key. Please
> see
> > src\Plugin\AdblockPlus.rgs for more info 
> 
> This is for Windows 8?

[Eric, 2014-07-18 16:40:45]

See review for "Whole installer"
http://codereview.adblockplus.org/6202981292703744/