Use libadblockplus update checker

This has been tested and works fine. The TODO comments refer to a dictionary access that needs to be shared between plugin and engine, as well as a progress indicator that requires changes to the WebRequest interface.

[Wladimir Palant, 2013-06-06 15:22:53]

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.h
File src/engine/Updater.h (right):

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.h#newcode14
src/engine/Updater.h:14: void RunDownload();
I don't think that the two functions above still need to be public, that's a
left-over from some changes.

[Felix Dahlke, 2013-06-06 17:44:00]

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.cpp
File src/engine/Updater.cpp (right):

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.cpp#newc...
src/engine/Updater.cpp:57: CallbackType* callback =
reinterpret_cast<CallbackType*>(param);
I'd use an auto_ptr. What if there's an exception in the callback?

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.cpp#newc...
src/engine/Updater.cpp:126: EndDialog(dialog, -1);
How about using a constant for this result? downloadFailed or something.

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.cpp#newc...
src/engine/Updater.cpp:154: return;
Not really necessary

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.h
File src/engine/Updater.h (right):

http://codereview.adblockplus.org/10800092/diff/1/src/engine/Updater.h#newcode14
src/engine/Updater.h:14: void RunDownload();
On 2013/06/06 15:22:53, Wladimir Palant wrote:
> I don't think that the two functions above still need to be public, that's a
> left-over from some changes.

Don't think so either :)

[Wladimir Palant, 2013-06-07 05:30:43]

I addressed all comments made by Felix and a few other minor issues.

[Felix Dahlke, 2013-06-07 07:35:01]

LGTM

[Oleksandr, 2013-06-07 08:37:44]

http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp
File src/engine/Updater.cpp (right):

http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp#n...
src/engine/Updater.cpp:77: : jsEngine(jsEngine), url(url),
tempFile(GetAppDataPath() + L"\\update.exe")
What if we decide we'd like to go with an MSI installer?

http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp#n...
src/engine/Updater.cpp:109: if (!::CreateProcessW(tempFile.c_str(), NULL, NULL,
NULL, FALSE, CREATE_BREAKAWAY_FROM_JOB, NULL, NULL, &si, &pi))
Again, what if we download an msi? Then we'll need to do msiexec.exe /i tempfile

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

http://codereview.adblockplus.org/10800092/diff/7001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:104: settings->SetString(SETTING_PLUGIN_VERSION,
IEPLUGIN_VERSION);
Looks like the whole "if" can be removed here. We do not store an updated plugin
version anywhere anyway, it seems.

[Wladimir Palant, 2013-06-07 12:46:55]

http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp
File src/engine/Updater.cpp (right):

http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp#n...
src/engine/Updater.cpp:77: : jsEngine(jsEngine), url(url),
tempFile(GetAppDataPath() + L"\\update.exe")
On 2013/06/07 08:37:44, Oleksandr wrote:
> What if we decide we'd like to go with an MSI installer?

Given the limitations of a pure MSI solution - we won't.

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

http://codereview.adblockplus.org/10800092/diff/7001/src/plugin/PluginClass.c...
src/plugin/PluginClass.cpp:104: settings->SetString(SETTING_PLUGIN_VERSION,
IEPLUGIN_VERSION);
On 2013/06/07 08:37:44, Oleksandr wrote:
> Looks like the whole "if" can be removed here. We do not store an updated
plugin
> version anywhere anyway, it seems.

Yep, lots of code can be removed here - I am just not sure which parts of it are
still used. I've tried to remove code only minimally here, we probably can
remove IsFirstRunUpdate() and everything dependent on it - cleaning up is on the
tasks list for the release.

[Oleksandr, 2013-06-07 12:49:33]

On 2013/06/07 12:46:55, Wladimir Palant wrote:
> http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp
> File src/engine/Updater.cpp (right):
> 
>
http://codereview.adblockplus.org/10800092/diff/7001/src/engine/Updater.cpp#n...
> src/engine/Updater.cpp:77: : jsEngine(jsEngine), url(url),
> tempFile(GetAppDataPath() + L"\\update.exe")
> On 2013/06/07 08:37:44, Oleksandr wrote:
> > What if we decide we'd like to go with an MSI installer?
> 
> Given the limitations of a pure MSI solution - we won't.
> 
>
http://codereview.adblockplus.org/10800092/diff/7001/src/plugin/PluginClass.cpp
> File src/plugin/PluginClass.cpp (right):
> 
>
http://codereview.adblockplus.org/10800092/diff/7001/src/plugin/PluginClass.c...
> src/plugin/PluginClass.cpp:104: settings->SetString(SETTING_PLUGIN_VERSION,
> IEPLUGIN_VERSION);
> On 2013/06/07 08:37:44, Oleksandr wrote:
> > Looks like the whole "if" can be removed here. We do not store an updated
> plugin
> > version anywhere anyway, it seems.
> 
> Yep, lots of code can be removed here - I am just not sure which parts of it
are
> still used. I've tried to remove code only minimally here, we probably can
> remove IsFirstRunUpdate() and everything dependent on it - cleaning up is on
the
> tasks list for the release.


I can handle the cleanup later then. LGTM