Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
Aug. 8, 2014, 9:01 a.m. by Oleksandr
Modified:
Aug. 8, 2014, 6:25 p.m.
Reviewers:
sergei, Felix Dahlke, Eric
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
Aug. 8, 2014, 9:07 a.m. (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 ...
Aug. 8, 2014, 2:14 p.m. (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 ...
Aug. 8, 2014, 3:17 p.m. (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 ...
Aug. 8, 2014, 3:34 p.m. (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 ...
Aug. 8, 2014, 3:45 p.m. (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 ...
Aug. 8, 2014, 4:29 p.m. (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? ...
Aug. 8, 2014, 4:37 p.m. (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 ...
Aug. 8, 2014, 5:16 p.m. (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 ...
Aug. 8, 2014, 5:43 p.m. (2014-08-08 17:43:58 UTC) #9
Eric
Aug. 8, 2014, 6:25 p.m. (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.

Powered by Google App Engine
This is Rietveld