Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(168)

Issue 5643369099296768: Issue 1147 - Fix installation when IE is opened (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 2 months ago by Oleksandr
Modified:
5 years, 2 months ago
Reviewers:
Eric, sergei, Felix Dahlke
Visibility:
Public.

Description

This does in fact fix the installer workflow - it works (like Eric suggested). However the debug message about leaked MSIHANDLEs still persists. I have created a separate issue for that https://issues.adblockplus.org/ticket/1187 for installer cleanup project.

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -2 lines) Patch
M installer/src/installer-lib/process.cpp View 2 chunks +9 lines, -2 lines 6 comments Download

Messages

Total messages: 10
Oleksandr
5 years, 2 months ago (2014-08-08 09:07:32 UTC) #1
Eric
LGTM, because the one issue I have is not a defect. It's just a bit ...
5 years, 2 months ago (2014-08-08 14:14:58 UTC) #2
sergei
Despite I'm not a reviewer here and it's already committed, I will leave my comment ...
5 years, 2 months ago (2014-08-08 15:17:36 UTC) #3
Eric
http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/installer/src/installer-lib/process.cpp File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/installer/src/installer-lib/process.cpp#newcode25 installer/src/installer-lib/process.cpp:25: if ((procHandle == NULL) || (procHandle == INVALID_HANDLE_VALUE)) return ...
5 years, 2 months ago (2014-08-08 15:34:18 UTC) #4
sergei
http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/installer/src/installer-lib/process.cpp File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/5643369099296768/diff/5629499534213120/installer/src/installer-lib/process.cpp#newcode36 installer/src/installer-lib/process.cpp:36: Windows_Module_Handle user32Dll(tmpModule); > These handle classes throw an exception ...
5 years, 2 months ago (2014-08-08 15:45:24 UTC) #5
Eric
On 2014/08/08 15:45:24, sergei wrote: > But why? Because the template is instantiated with a ...
5 years, 2 months ago (2014-08-08 16:29:35 UTC) #6
Felix Dahlke
On 2014/08/08 16:29:35, Eric wrote: > On 2014/08/08 15:45:24, sergei wrote: > > But why? ...
5 years, 2 months ago (2014-08-08 16:37:52 UTC) #7
Eric
On 2014/08/08 16:37:52, Felix H. Dahlke wrote: > I don't understand why you react this ...
5 years, 2 months ago (2014-08-08 17:16:23 UTC) #8
Felix Dahlke
Some more comments below. As for this review, it's LGTM from my side. We should ...
5 years, 2 months ago (2014-08-08 17:43:58 UTC) #9
Eric
5 years, 2 months ago (2014-08-08 18:25:49 UTC) #10
On 2014/08/08 17:43:58, Felix H. Dahlke wrote:
> IMO that would have made more sense here - the value CAN be null, that way we
> could check for it without the temporary variable.

I'm not going to disagree. Windows_Handle was used in only in a single place in
the code previously, and it was advantageous to declare it not-null to simplify
the code of its first usage. As a rule, reference-style classes lead to cleaner
code, since many objects tend to be used more than they're constructed. In that
case of, say, a factory function, checking for null explicitly before
construction isolates all the null checking in that one place, and it eliminates
it everywhere else.

The present case it's used as a transient value, and using a reference-style
declaration is slightly wordier.

> I was talking about documentation of our own code. Consistency can help a lot
> here: We already have a handle wrapper that does handle null as a value.
Ideally
> we would just use that one here.

Windows_Handle was needed after the handle generic was already written. It was
very quick to make it work.

typedef handle< HANDLE, Disallow_Null, Windows_Generic_Destruction >
Windows_Handle

I think I had to write Windows_Generic_Destruction, but that's close to a
one-liner around CloseHandle(...).

> I think it'd be helpful to have two typedefs
> for that, let's say: WindowsHandle and NonnullWindowsHandle. Since handles can
> be null, I intuitively expect a handle wrapper to handle (pun intended) that
> too. Both Sergei and I assumed this, I believe that backs the point up.

typedef handle< HANDLE, Allow_Null, Windows_Generic_Destruction > WindowsHandle
typedef handle< HMODULE, Allow_Null, Windows_Generic_Destruction > ModuleHandle
typedef handle< HANDLE, Disallow_Null, Windows_Generic_Destruction >
WindowsHandleRef

Probably easier to just to make these declarations. Frankly, you don't want
something named AutoHandle in the installer code because there are so many
different kinds of handles running around; AutoHandle just isn't specific enough
a name.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5