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

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

Aug. 8, 2014, 9:01 a.m. by Oleksandr
Aug. 8, 2014, 6:25 p.m.
sergei, Felix Dahlke, Eric


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


Total messages: 10
Aug. 8, 2014, 9:07 a.m. (2014-08-08 09:07:32 UTC) #1
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
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
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
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
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
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
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.
> 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 >

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 >

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