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

Issue 6689082285490176: Issue 1598 - The set up was unable to automatically close IE while uninstall

Created:
March 18, 2015, 1:42 a.m. by Oleksandr
Modified:
June 15, 2015, 7:29 a.m.
Reviewers:
sergei, Eric
CC:
Felix Dahlke
Visibility:
Public.

Description

1. Custom action is not being run at all on uninstall now, since it is set in InstallUISequence. Here we set it for InstallExecuteSequence. In this case the custom action will run right before the actual installation, after the user clicks the last "Next". Which is a better place to run it too, IMO. 2. I would remove the partKnown state altogether. Right now we are effectively asking the user 2 times if s/he would like to close IE. For now, since the uninstall runs with /qb let's default to partKnown, so that the installer would close IE on user's consent.

Patch Set 1 #

Total comments: 2

Patch Set 2 : Treat Basic UI as interactive mode #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -4 lines) Patch
M installer/src/custom-action/close_application.cpp View 1 1 chunk +2 lines, -2 lines 0 comments Download
M installer/src/msi/adblockplusie.wxs View 2 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 14
Oleksandr
March 18, 2015, 1:52 a.m. (2015-03-18 01:52:20 UTC) #1
Eric
Not LGTM. There's a basic bad design in the Windows Installer, which is that you ...
March 18, 2015, 4:13 p.m. (2015-03-18 16:13:47 UTC) #2
Eric
That shouldn't have shown up green. I thought "N-o-t L-G-T-M" highlighted differently. Maybe it should ...
March 18, 2015, 4:15 p.m. (2015-03-18 16:15:07 UTC) #3
Eric
Oh well. NOT GREEN.
March 18, 2015, 4:15 p.m. (2015-03-18 16:15:23 UTC) #4
Eric
As far as the "partKnown" state goes, that's primarily a workaround of the very limited ...
March 18, 2015, 4:19 p.m. (2015-03-18 16:19:04 UTC) #5
Oleksandr
Let's take off our engineer hats and use our user hats for a moment. I ...
March 18, 2015, 6:19 p.m. (2015-03-18 18:19:55 UTC) #6
Eric
On 2015/03/18 18:19:55, Oleksandr wrote: > The reason behind this change-set is a bad user ...
March 18, 2015, 6:53 p.m. (2015-03-18 18:53:45 UTC) #7
Oleksandr
I think the core question here is this: what should be an interactive mode and ...
March 19, 2015, 2:14 a.m. (2015-03-19 02:14:46 UTC) #8
Oleksandr
March 19, 2015, 2:20 a.m. (2015-03-19 02:20:11 UTC) #9
Eric
On 2015/03/19 02:14:46, Oleksandr wrote: > In the code and in the comments here you ...
March 19, 2015, 12:42 p.m. (2015-03-19 12:42:20 UTC) #10
Oleksandr
I have tested this on Windows XP as well and it worked the same way. ...
March 20, 2015, 5:13 a.m. (2015-03-20 05:13:05 UTC) #11
Eric
On 2015/03/20 05:13:05, Oleksandr wrote: > I have tested this on Windows XP as well ...
March 20, 2015, 12:11 p.m. (2015-03-20 12:11:52 UTC) #12
Oleksandr
Tested under XP running an uninstall from the command line ("msiexec /x") > separately with ...
March 20, 2015, 7:37 p.m. (2015-03-20 19:37:33 UTC) #13
Eric
March 20, 2015, 8:10 p.m. (2015-03-20 20:10:01 UTC) #14
On 2015/03/20 19:37:33, Oleksandr wrote:
> No. Uninstall from control panel launches in Basic UI. ie /qb

That's news to me.

And /qb isn't the only way of suppressing the default UI. You can make authored
UI in the MSI tables contingent upon conditions.

-----

What this change set needs, desperately, is enough documentation and commentary
that someone who doesn't know all the intricacies of Windows Installer to be
able to audit the code and determine that it's behaving correctly. Such writing
would need to examine all the cases where the CA executes, not just the most
common ones. We have .dox files available for such writing; it doesn't all need
to be in comments.

I don't have anything new to say. I remain unconvinced that this code works
right.

Perhaps if I see enough writing on it I might be persuaded. That hasn't happened
yet.

Powered by Google App Engine
This is Rietveld