Issue 1103 - Migrate Simple Adblock users

1. This should probably be committed into a branch.
2. This adds the mechanics for the upgrade from Simple Adblock to Adblock Plus. The actual FRP changes will come later. Currently only the title is different.
3. The idea is to detect if Simple Adblock is installed in the installer. If is is then:
a) remove it
b) remove leftover files from "%APPDATA%\LocalLow\Simple Adblock"
c) deliver the prefs.json with {currentVersion: "simpleadblock"}. 
d) On first start, in the plugin, detect if there was an upgrade from Simple Adblock based on currentVersion.

[Eric, 2015-03-27 12:51:45]

Overall, I'm uncomfortable with sprinkling a lot of calls entirely specific to
Simple Adblock all over the code. It's a burden to carry around that code in the
plugin going forward for what is a transient period. Since we do need something
like it, though, the code will read better if the code is not specific to SA but
could be used for other products. This structure documents itself more clearly,
as well, since the function names would describe their purpose more clearly, not
being constrained to have SA in their names.

You had mentioned that we are talking about a separate MSI for upgrading SA. Is
the modification to WiX source going in the main MSI or a separate one? Without
seeing the changes to the build system, I can't evaluate changes in the MSI
completely, since it matters which artifact they'll appear in.

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/html...
File html/static/js/firstRun.js (right):

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/html...
html/static/js/firstRun.js:108: "title-main": AdblockPlus.isFromSimpleAdblock()
? "first-run-title-simple-adblock" :
It seems that the string "first-run-title-simple-adblock" could be a parameter,
perhaps part of the return value from the settings call.

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/html...
File html/static/js/ieFirstRun.js (right):

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/html...
html/static/js/ieFirstRun.js:43: result.isFromSimpleAdblock = function()
If we called this isConversion, we could return a null string meaning "no
upgrade" or an identifying string naming the product we are converting from.

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

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:137: Schedule="afterInstallInitialize"
Please see the comment immediately about about why the 'Schedule' attribute was
set that way. Getting rid of atomicity is asking for trouble.

I don't see any documentation about why this would be scheduled differently.

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:189: ExeCommand="cmd /C "rmdir /s /q
".\Simple Adblock"""
Using "." for current directory is likely not to be as reliable as we'd like.

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:743:
CPluginClient::GetInstance()->SetPref(L"currentVersion",
std::wstring(IEPLUGIN_VERSION));
Instead of overloading "currentVersion", is it impossible for us to use a new
preference and delete it here?

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
File src/plugin/PluginUserSettings.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
src/plugin/PluginUserSettings.cpp:70: m.emplace(L"IsFromSimpleAdblock",
dispatchID_IsFromSimpleAdblock);
I would prefer an API here that's a bit more general. Instead of something
that's only about Simple Adblock, we could use a function that takes argument
"simpleadblock"?

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
src/plugin/PluginUserSettings.cpp:456: pVarResult->boolVal =
CPluginClient::GetInstance()->GetPref(L"isFromSimpleAdblock", false) ?
VARIANT_TRUE : VARIANT_FALSE;
The preference could be called "upgradingFrom".

[Oleksandr, 2015-04-16 09:58:09]

After these changes I now think that it would be best to just release one msi
for all cases. So there will not be a separate msi just for migration.

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/html...
File html/static/js/firstRun.js (right):

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/html...
html/static/js/firstRun.js:108: "title-main": AdblockPlus.isFromSimpleAdblock()
? "first-run-title-simple-adblock" :
On 2015/03/27 12:51:45, Eric wrote:
> It seems that the string "first-run-title-simple-adblock" could be a
parameter,
> perhaps part of the return value from the settings call.

We have to hardcode this logic somewhere. I think it's better to do it in .js

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

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:137: Schedule="afterInstallInitialize"
On 2015/03/27 12:51:45, Eric wrote:
> Please see the comment immediately about about why the 'Schedule' attribute
was
> set that way. Getting rid of atomicity is asking for trouble.
> 
> I don't see any documentation about why this would be scheduled differently.

ABP for IE and SimpleAdblock have the same CLSID when registering into COM. When
uninstalling Simple Adblock the registry key gets removed, so this is why we
want to uninstall it before installing ABP for IE. What can possibly go wrong if
we run on afterInstallInitialize? The worst thing I can think of is that old
version gets uninstalled and new version fails to install. Is that it?

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:189: ExeCommand="cmd /C "rmdir /s /q
".\Simple Adblock"""
On 2015/03/27 12:51:45, Eric wrote:
> Using "." for current directory is likely not to be as reliable as we'd like.

Why not? We are launching in APP_DATA_FOLDER

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:743:
CPluginClient::GetInstance()->SetPref(L"currentVersion",
std::wstring(IEPLUGIN_VERSION));
On 2015/03/27 12:51:45, Eric wrote:
> Instead of overloading "currentVersion", is it impossible for us to use a new
> preference and delete it here?

We can only use one of the default prefs

[sergei, 2015-04-22 10:37:08]

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

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:137: Schedule="afterInstallInitialize"
On 2015/04/16 09:58:09, Oleksandr wrote:
> On 2015/03/27 12:51:45, Eric wrote:
> > Please see the comment immediately about about why the 'Schedule' attribute
> was
> > set that way. Getting rid of atomicity is asking for trouble.
> > 
> > I don't see any documentation about why this would be scheduled differently.
> 
> ABP for IE and SimpleAdblock have the same CLSID when registering into COM.
When
> uninstalling Simple Adblock the registry key gets removed, so this is why we
> want to uninstall it before installing ABP for IE. What can possibly go wrong
if
> we run on afterInstallInitialize? The worst thing I can think of is that old
> version gets uninstalled and new version fails to install. Is that it?
"afterInstallInitialize" still implies that uninstalling can be rolled back if
either installing or uninstalling fails because "InstallInitialize" action marks
the beginning of the "deferred" stage of the Execute sequence.

I would also like to see edited the comment regarding `Schedule` attribute. It
should explain that in comparison with the default value "afterInstallValidate"
(which can lead to a state without any version installed) we use
"afterInstallInitialize", so it's executed at the deferred stage and can be
rolled back in case of failure either of uninstalling (RemoveExitingProducts
standard action) or installing. As well as it can not be after
"afterInstallExecute" because in this case uninstalling would remove important
registry keys (in particular COM CLSID).

PS: I have not checked how we create registry keys yet, however I would expect
them to be not removed for Schedule="afterInstallExecute" because reference
counter should prevent it. But anyway I think that
Schedule="afterInstallInitialize" is safer because in this case the whole
procedure looks more straightforward and not affected by actions (in particular
possible custom actions) which don't use reference counters.

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/html...
File html/static/js/ieFirstRun.js (right):

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/html...
html/static/js/ieFirstRun.js:43: result.getConverssion = function(pluginName)
argument is not used

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

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/inst...
installer/src/custom-action/close_application.cpp:214: else if (uilevel == L"2")
it seems not a part of this issue.

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/src/...
src/plugin/PluginClass.cpp:858: std::wstring curVer =
CPluginClient::GetInstance()->GetPref(std::wstring(L"currentVersion"),
std::wstring(L"0.0"));
How does "0.0" affect `versionCompRes`? I'm not that it's it's a correct value,
should we use a real value of the current version?

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/src/...
src/plugin/PluginClass.cpp:859: if (curVer.find(L".") == std::wstring::npos)
I would live a comment here about "simpleadblock" case without point.

[sergei, 2015-04-22 14:11:43]

BTW, are you sure that it works, last time I tried to supply custom prefs.json
in tests and I discovered that initialization of prefs is slightly broken
because of race condition. Here it seems possible that firstly you will receive
"0.0" from `defaults` (see prefs.js) and make a decision that it's not 
migration from SA meanwhile a bit later the callback `_fileSystem.read(path,
function(result)` (due to asynchrony of prefs.json reading) will be called.

[Oleksandr, 2015-04-24 11:50:04]

On 2015/04/22 14:11:43, sergei wrote:
> BTW, are you sure that it works, last time I tried to supply custom prefs.json
> in tests and I discovered that initialization of prefs is slightly broken
> because of race condition. Here it seems possible that firstly you will
receive
> "0.0" from `defaults` (see prefs.js) and make a decision that it's not 
> migration from SA meanwhile a bit later the callback `_fileSystem.read(path,
> function(result)` (due to asynchrony of prefs.json reading) will be called.

Can you share the test case that reproduces the race condition? As I see it
Prefs are read right on load, not on any event. So since JavaScript is single
threaded, a call to getPref is sure to be executed after the Prefs were read.
Only if Prefs are invalid or non-existent then the Prefs initialization would
run on event.

[Oleksandr, 2015-04-24 11:50:31]

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

http://codereview.adblockplus.org/5024350814076928/diff/5629499534213120/inst...
installer/src/msi/adblockplusie.wxs:137: Schedule="afterInstallInitialize"
On 2015/04/22 10:37:08, sergei wrote:
> On 2015/04/16 09:58:09, Oleksandr wrote:
> > On 2015/03/27 12:51:45, Eric wrote:
> > > Please see the comment immediately about about why the 'Schedule'
attribute
> > was
> > > set that way. Getting rid of atomicity is asking for trouble.
> > > 
> > > I don't see any documentation about why this would be scheduled
differently.
> > 
> > ABP for IE and SimpleAdblock have the same CLSID when registering into COM.
> When
> > uninstalling Simple Adblock the registry key gets removed, so this is why we
> > want to uninstall it before installing ABP for IE. What can possibly go
wrong
> if
> > we run on afterInstallInitialize? The worst thing I can think of is that old
> > version gets uninstalled and new version fails to install. Is that it?
> "afterInstallInitialize" still implies that uninstalling can be rolled back if
> either installing or uninstalling fails because "InstallInitialize" action
marks
> the beginning of the "deferred" stage of the Execute sequence.
> 
> I would also like to see edited the comment regarding `Schedule` attribute. It
> should explain that in comparison with the default value
"afterInstallValidate"
> (which can lead to a state without any version installed) we use
> "afterInstallInitialize", so it's executed at the deferred stage and can be
> rolled back in case of failure either of uninstalling (RemoveExitingProducts
> standard action) or installing. As well as it can not be after
> "afterInstallExecute" because in this case uninstalling would remove important
> registry keys (in particular COM CLSID).
> 
> PS: I have not checked how we create registry keys yet, however I would expect
> them to be not removed for Schedule="afterInstallExecute" because reference
> counter should prevent it. But anyway I think that
> Schedule="afterInstallInitialize" is safer because in this case the whole
> procedure looks more straightforward and not affected by actions (in
particular
> possible custom actions) which don't use reference counters.

SimpleAdblock uninstall removes the registry keys using DllUnregisterServer, so
installer is not managing those.

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

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/inst...
installer/src/custom-action/close_application.cpp:214: else if (uilevel == L"2")
On 2015/04/22 10:37:09, sergei wrote:
> it seems not a part of this issue.

Yes, this is part of http://codereview.adblockplus.org/6689082285490176/

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/src/...
src/plugin/PluginClass.cpp:858: std::wstring curVer =
CPluginClient::GetInstance()->GetPref(std::wstring(L"currentVersion"),
std::wstring(L"0.0"));
On 2015/04/22 10:37:09, sergei wrote:
> How does "0.0" affect `versionCompRes`? I'm not that it's it's a correct
value,
> should we use a real value of the current version?

I don't think we should default to current version. If a version can't be
detected then it's better to play safe and make sure we show the FRP.

[sergei, 2015-04-28 14:30:29]

On 2015/04/24 11:50:04, Oleksandr wrote:
> call to getPref is sure to be executed after the Prefs were read.
Yes, you are right, sorry for confusing here. However I am going to add the
explanation of the test case.

Here it works because we use `FilterEngine::GetPref`, the important part is that
it's a member of FilterEngine which waits in the constructor for finishing of
loading of Prefs (see prefs.js, init.js and FilterEngine.cpp). However when one
accesses Prefs from javascript, prefs can be not loaded yet, so we cannot rely
on prefs during initialization of modules (e.g. it would be helpful to inject
INITIAL_DELAY and other parameters into updater.js, notification.js and so on)
and in functions which are called by timer while settings can be still not
loaded.

[sergei, 2015-04-28 14:41:14]

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

http://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/inst...
installer/src/custom-action/close_application.cpp:214: else if (uilevel == L"2")
On 2015/04/24 11:50:32, Oleksandr wrote:
> On 2015/04/22 10:37:09, sergei wrote:
> > it seems not a part of this issue.
> 
> Yes, this is part of http://codereview.adblockplus.org/6689082285490176/

So, do you mind commit it separately?

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
File src/plugin/PluginUserSettings.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
src/plugin/PluginUserSettings.cpp:456: pVarResult->bstrVal =
CComBSTR(CPluginClient::GetInstance()->GetPref(std::wstring(L"convertedFrom"),
std::wstring(L"")).c_str());
Is it necessary to explicitly construct std::wstring here?

[Eric, 2015-05-17 00:29:24]

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/html...
File html/static/js/firstRun.js (right):

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/html...
html/static/js/firstRun.js:131: if (AdblockPlus.isUpgradeFrom("simpleadblock"))
{
Where is 'isUpgradeFrom' defined?

Is it the same as 
  AdblockPlus.getConversion() == "simpleadblock"
as above?

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/html...
File html/static/js/ieFirstRun.js (right):

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/html...
html/static/js/ieFirstRun.js:45: return Settings.GetConverssion();
Typo. 'GetConversion()'. One 's'.

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

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/inst...
installer/src/msi/adblockplusie.wxs:141: Schedule="afterInstallInitialize"
This change remains a bad idea. It's altering the scheduling for all ordinary
ABP upgrades in order to handle a small set of transient cases. We don't have to
make this change in order to accomplish the stated goal. There are other MSI
mechanisms to deal with it. 

Since the problem is that we need to uninstall SimpleAdblock first, we can do
exactly that without altering the existing sequence for major upgrade. The
'RemoveExistingProducts' action can be rescheduled to run earlier. This action
only affects products that occur within the upgrade table, which for us is only
SimpleAdblock.

https://msdn.microsoft.com/en-us/library/aa371197(v=vs.85).aspx
http://wixtoolset.org/documentation/manual/v3/xsd/wix/removeexistingproducts....

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/inst...
installer/src/msi/adblockplusie.wxs:146: <Property Id="SIMPLEADBLOCKDETECTED"
Secure="yes"/>
The value of this property is set by the 'FindRelatedProducts' action. It is set
to the product code, if found. A better name would be
"SIMPLEADBLOCKPRODUCTCODE".

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/inst...
installer/src/msi/adblockplusie.wxs:203: <InstallExecuteSequence>
Moving the close-IE action should be done in a separate commit. We can have both
tags, one for each UI and Execute sequences.

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
src/plugin/PluginClass.cpp:858: std::wstring curVer =
CPluginClient::GetInstance()->GetPref(std::wstring(L"currentVersion"),
std::wstring(L"0.0"));
Don't need explicit conversions to 'std::wstring'

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
src/plugin/PluginClass.cpp:874: if
(!CPluginClient::GetInstance()->GetPref(curVer, false))
Is the value of 'curVer' the name of a preference?

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
File src/plugin/PluginUserSettings.cpp (right):

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
src/plugin/PluginUserSettings.cpp:430:
client->RemoveSubscription(client->GetPref(L"subscriptions_exceptionsurl",
L""));
See here, for example, of why explicit conversions to 'std::wstring' aren't
necessary.

http://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/src/...
src/plugin/PluginUserSettings.cpp:456: pVarResult->bstrVal =
CComBSTR(CPluginClient::GetInstance()->GetPref(std::wstring(L"convertedFrom"),
std::wstring(L"")).c_str());
On 2015/04/28 14:41:15, sergei wrote:
> Is it necessary to explicitly construct std::wstring here?

It's not. There's an explicit pass through function for second argument
'wchar_t*' that invokes the 'std::wstring' version.

[Oleksandr, 2015-07-13 03:46:16]

https://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/ins...
File installer/src/msi/adblockplusie.wxs (right):

https://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/ins...
installer/src/msi/adblockplusie.wxs:141: Schedule="afterInstallInitialize"
On 2015/05/17 00:29:25, Eric wrote:
> This change remains a bad idea. It's altering the scheduling for all ordinary
> ABP upgrades in order to handle a small set of transient cases. We don't have
to
> make this change in order to accomplish the stated goal. There are other MSI
> mechanisms to deal with it. 
> 
> Since the problem is that we need to uninstall SimpleAdblock first, we can do
> exactly that without altering the existing sequence for major upgrade. The
> 'RemoveExistingProducts' action can be rescheduled to run earlier. This action
> only affects products that occur within the upgrade table, which for us is
only
> SimpleAdblock.
> 
> https://msdn.microsoft.com/en-us/library/aa371197(v=vs.85).aspx
>
http://wixtoolset.org/documentation/manual/v3/xsd/wix/removeexistingproducts....

1. MajorUpgrade already schedules RemoveExistingProducts. We can't schedule it
separately, it seems.
2. Just to clarify: both afterInstallExecute and afterInstallInitialize happen
*after* a user clicks on the final "Install" button in the UI. I have tested
quite a few update scenarios, and I don't see any real problem in uninstalling
the previous ABP version before installing a new one. We can always change this
back to how it is now in the next release. This essentially has to be done for
just one release.

[Eric, 2015-07-27 02:44:55]

LGTM.

Small caveat. As the review notes point out, there's no first run page
specifically for this conversion. Therefore the suggestion to commit it to a
branch seems prudent, since this change does not stand alone with that page (and
all its translations).

[sergei, 2015-07-28 08:56:41]

https://codereview.adblockplus.org/5024350814076928/diff/29322045/html/static...
File html/static/js/ieFirstRun.js (left):

https://codereview.adblockplus.org/5024350814076928/diff/29322045/html/static...
html/static/js/ieFirstRun.js:43: result.getConverssion = function()
It does not seem to be the correct patch because the previous version is
different from the current version
https://hg.adblockplus.org/adblockplusie/file/f6bd11fbef97/html/static/js/ieF...

[Oleksandr, 2015-07-28 09:56:39]

Rebased.

https://codereview.adblockplus.org/5024350814076928/diff/29322045/html/static...
File html/static/js/ieFirstRun.js (left):

https://codereview.adblockplus.org/5024350814076928/diff/29322045/html/static...
html/static/js/ieFirstRun.js:43: result.getConverssion = function()
On 2015/07/28 08:56:40, sergei wrote:
> It does not seem to be the correct patch because the previous version is
> different from the current version
>
https://hg.adblockplus.org/adblockplusie/file/f6bd11fbef97/html/static/js/ieF...

You're right. Rebased.

[sergei, 2015-07-28 11:11:55]

LGTM

https://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/ins...
File installer/src/custom-action/close_application.cpp (right):

https://codereview.adblockplus.org/5024350814076928/diff/5653164804014080/ins...
installer/src/custom-action/close_application.cpp:214: else if (uilevel == L"2")
On 2015/04/28 14:41:15, sergei wrote:
> On 2015/04/24 11:50:32, Oleksandr wrote:
> > On 2015/04/22 10:37:09, sergei wrote:
> > > it seems not a part of this issue.
> > 
> > Yes, this is part of http://codereview.adblockplus.org/6689082285490176/
> 
> So, do you mind commit it separately?

So, it's still here, is it essential for this issue?

https://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/ins...
File installer/src/msi/adblockplusie.wxs (right):

https://codereview.adblockplus.org/5024350814076928/diff/5631943370604544/ins...
installer/src/msi/adblockplusie.wxs:203: <InstallExecuteSequence>
On 2015/05/17 00:29:25, Eric wrote:
> Moving the close-IE action should be done in a separate commit. We can have
both
> tags, one for each UI and Execute sequences.

Actually yes, had someone created an issue for it?

[Oleksandr, 2015-08-07 10:41:01]

Since https://codereview.adblockplus.org/6505394822184960/ will give us an
engine that is always in Medium Integrity, and storing the Simple Adblock mark
in prefs was a hack, it would be better to store the information about Simple
Adblock update in the registry instead. Added this change it the latest patch.

[Eric, 2015-08-10 17:08:50]

https://codereview.adblockplus.org/5024350814076928/diff/29323392/src/engine/...
File src/engine/Main.cpp (right):

https://codereview.adblockplus.org/5024350814076928/diff/29323392/src/engine/...
src/engine/Main.cpp:388: LONG res = RegDeleteKey(HKEY_CURRENT_USER,
updatedFromKeyPath.c_str());
Deleting the key immediately after returning its status means that the
combination of 
  (1) display conversion page and 
  (2) delete the flag that mandates the display
is not an atomic action. In other words if there's an error displaying the page,
or a _de facto_ failure for the user to see it, such as immediate shutdown, then
we don't get another chance to show the page again.

If I understand the comment that introduced this patch set, this change is
possible because a medium integrity engine can delete the registry key whereas a
low integrity one cannot. (This point should be commented in the code, also.)
Assuming that's right, all we need in the engine is to be able to delete the
key. We can still read it in the plugin, where we'd also have access to any
display error.

If we didn't need medium integrity to delete such a key, we wouldn't need this
piece in the engine at all. It could all go in the plugin. This is a design
point worth documenting in the code. I wouldn't consider it obvious, since these
permission are ambient (not directly visible in the code) and their behavior
with respect to the registry is something most don't concern themselves with.

I do understand that there's JavaScript code involved as well, so it's not quite
as simple as I'm making it out, but it's still a design problem to bind query
and delete together as it is here.