Download updates automatically, ask to install from a modeless dialog

This is what now happens when an update is available:

1. Download the update silently
2. Ask the user if it should be installed
3. Install it

Step two is now a modeless dialog with a thin titlebar that appears in the lower right corner of the screen, similar to desktop notifications in Firefox on Windows.

[Oleksandr, 2013-07-31 22:16:59]

This looks cool for the purpose of just showing the Update Dialog. However I was
looking forward for something more generic. Something that would be able to
accept strings from the named pipe and display it to the user. I've indicated in
comments a possible way of making it more fitting to my vision. Please feel free
to suggest other options.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
File src/engine/UpdateInstallDialog.cpp (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:52: std::wstring title =
dictionary->Lookup("updater", "install-question-title");
This is fine only for when download is ready. How about also having an
overridden constructor that would be something like:
UpdateInstallDialog::UpdateInstallDialog(std::wstring title, std::wstring
message)?

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:67: CreateButtons();
No CreateButtons() in the overridden constructor I suggest. Or maybe something
like CreateOKButton().

[Felix Dahlke, 2013-07-31 22:20:35]

On 2013/07/31 22:16:59, Oleksandr wrote:
> This looks cool for the purpose of just showing the Update Dialog. However I
was
> looking forward for something more generic. Something that would be able to
> accept strings from the named pipe and display it to the user.

Why would we want to have the plugin show messages to the user? Seems like it's
fine if the engine is in charge of that. I did not account for the "Sorry, no
updates available" message here, but that's because we're not 100% sure how to
show that yet, I don't like to write unused code. It's fairly easy to implement
based on this though, we can indeed make UpdateInstallDialog more generic for
that (optional buttons, as you suggsted).

[Oleksandr, 2013-07-31 22:36:51]

I think you're right - currently we don't need to show anything from the plugin
to user. I am just not quite sure if there is "Already up to date" event from
the javascript code. 
I was thinking it might be nice to have this option for future (reporting a bad
website behaviour? Or "this website runs on ads, which we've blocked. click here
to support it directly", or "oops, IE has crashed. >report a bug here<" :) 
But I guess it isn't really needed now, (or ever) and having the engine totally
taking care of that makes more sense.

On 2013/07/31 22:20:35, Felix H. Dahlke wrote:
> On 2013/07/31 22:16:59, Oleksandr wrote:
> > This looks cool for the purpose of just showing the Update Dialog. However I
> was
> > looking forward for something more generic. Something that would be able to
> > accept strings from the named pipe and display it to the user.
> 
> Why would we want to have the plugin show messages to the user? Seems like
it's
> fine if the engine is in charge of that. I did not account for the "Sorry, no
> updates available" message here, but that's because we're not 100% sure how to
> show that yet, I don't like to write unused code. It's fairly easy to
implement
> based on this though, we can indeed make UpdateInstallDialog more generic for
> that (optional buttons, as you suggsted).

[Oleksandr, 2013-08-01 07:05:11]

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
File src/engine/UpdateInstallDialog.cpp (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:42: windowClass.lpszClassName =
windowClassName.c_str();
Maybe use hIcon here with ABP logo? A default window icon popping up out of the
blue isn't exactly a good user experience, I think.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:65: width, height, 0, 0, 0, 0);
Should we do something like:

SetForegroundWindow(window);

It will flash the window in the taskbar, if the user is working in another
window. So that the user will notice it.

[Felix Dahlke, 2013-08-01 07:21:17]

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
File src/engine/UpdateInstallDialog.cpp (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:42: windowClass.lpszClassName =
windowClassName.c_str();
On 2013/08/01 07:05:12, Oleksandr wrote:
> Maybe use hIcon here with ABP logo? A default window icon popping up out of
the
> blue isn't exactly a good user experience, I think.

Does it show any icons for you? It doesn't even have an entry in the task bar
for me.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:52: std::wstring title =
dictionary->Lookup("updater", "install-question-title");
On 2013/07/31 22:16:59, Oleksandr wrote:
> This is fine only for when download is ready. How about also having an
> overridden constructor that would be something like:
> UpdateInstallDialog::UpdateInstallDialog(std::wstring title, std::wstring
> message)? 

Like I said earlier, I think it's better to focus on what we want to do for now,
not on what we could do. We can always change this later.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:65: width, height, 0, 0, 0, 0);
On 2013/08/01 07:05:12, Oleksandr wrote:
> Should we do something like:
> 
> SetForegroundWindow(window);
> 
> It will flash the window in the taskbar, if the user is working in another
> window. So that the user will notice it.

Well, the window is already WS_TOPMOST, so it's kinda hard to miss :D

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:67: CreateButtons();
On 2013/07/31 22:16:59, Oleksandr wrote:
> No CreateButtons() in the overridden constructor I suggest. Or maybe something
> like CreateOKButton().

As I said above, let's make this more generic when we need that.

[Oleksandr, 2013-08-01 08:44:15]

LGTM as a first step for a more robust notification mechanism
On 2013/08/01 07:21:17, Felix H. Dahlke wrote:
>
http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
> File src/engine/UpdateInstallDialog.cpp (right):
> 
>
http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
> src/engine/UpdateInstallDialog.cpp:42: windowClass.lpszClassName =
> windowClassName.c_str();
> On 2013/08/01 07:05:12, Oleksandr wrote:
> > Maybe use hIcon here with ABP logo? A default window icon popping up out of
> the
> > blue isn't exactly a good user experience, I think.
> 
> Does it show any icons for you? It doesn't even have an entry in the task bar
> for me.
> 
>
http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
> src/engine/UpdateInstallDialog.cpp:52: std::wstring title =
> dictionary->Lookup("updater", "install-question-title");
> On 2013/07/31 22:16:59, Oleksandr wrote:
> > This is fine only for when download is ready. How about also having an
> > overridden constructor that would be something like:
> > UpdateInstallDialog::UpdateInstallDialog(std::wstring title, std::wstring
> > message)? 
> 
> Like I said earlier, I think it's better to focus on what we want to do for
now,
> not on what we could do. We can always change this later.
> 
>
http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
> src/engine/UpdateInstallDialog.cpp:65: width, height, 0, 0, 0, 0);
> On 2013/08/01 07:05:12, Oleksandr wrote:
> > Should we do something like:
> > 
> > SetForegroundWindow(window);
> > 
> > It will flash the window in the taskbar, if the user is working in another
> > window. So that the user will notice it.
> 
> Well, the window is already WS_TOPMOST, so it's kinda hard to miss :D
> 
>
http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
> src/engine/UpdateInstallDialog.cpp:67: CreateButtons();
> On 2013/07/31 22:16:59, Oleksandr wrote:
> > No CreateButtons() in the overridden constructor I suggest. Or maybe
something
> > like CreateOKButton().
> 
> As I said above, let's make this more generic when we need that.

[Wladimir Palant, 2013-08-01 09:58:24]

http://codereview.adblockplus.org/11304082/diff/10001/locales/en.ini
File locales/en.ini (right):

http://codereview.adblockplus.org/11304082/diff/10001/locales/en.ini#newcode14
locales/en.ini:14: install-question-text=A new version of Adblock Plus has been
downloaded. Update now?
How about leaving out the technical details:

A new version of Adblock Plus is ready to be installed. Do you want to install
it now?

http://codereview.adblockplus.org/11304082/diff/10001/locales/en.ini#newcode16
locales/en.ini:16: install-error-text=An error occurred while updating Adblock
Plus
Period at the end of this sentence?

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
File src/engine/UpdateInstallDialog.cpp (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:38: WNDCLASSEX windowClass = {};
We should use WNDCLASSEXW explicitly.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:39: windowClass.cbSize = sizeof(WNDCLASSEX);
sizeof(windowClass) to avoid repeating the type name?

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:58: const int height = 150;
Where do these numbers come from? What will happen on high-DPI screens or with
lengthy translations? I suggest you measure the text size to determine window
size.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:85: return message.wParam;
Doesn't this cause a warning? I would rather cast to bool explicitly here.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:116: const int buttonHeight = 30;
Same issue as with the text, hardcoded size will be problematic if font size
changes or translation is overly long.

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

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/Updater.cpp#...
src/engine/Updater.cpp:141: dictionary->Lookup("updater",
"install-error-title").c_str(), MB_OK);
Add MB_ICONEXCLAMATION flag?

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/engine.rc
File src/engine/engine.rc (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/engine.rc#ne...
src/engine/engine.rc:2: #include "Resource.h"
You should remove Resource.h - none of the IDs defined there are used.

[Felix Dahlke, 2013-08-02 10:48:01]

Uploaded a new patch set addressing all issues.

http://codereview.adblockplus.org/11304082/diff/10001/locales/en.ini
File locales/en.ini (right):

http://codereview.adblockplus.org/11304082/diff/10001/locales/en.ini#newcode14
locales/en.ini:14: install-question-text=A new version of Adblock Plus has been
downloaded. Update now?
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> How about leaving out the technical details:
> 
> A new version of Adblock Plus is ready to be installed. Do you want to install
> it now?

Done.

http://codereview.adblockplus.org/11304082/diff/10001/locales/en.ini#newcode16
locales/en.ini:16: install-error-text=An error occurred while updating Adblock
Plus
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> Period at the end of this sentence?

Done.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
File src/engine/UpdateInstallDialog.cpp (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:38: WNDCLASSEX windowClass = {};
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> We should use WNDCLASSEXW explicitly.

Done.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:39: windowClass.cbSize = sizeof(WNDCLASSEX);
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> sizeof(windowClass) to avoid repeating the type name?

Done.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:58: const int height = 150;
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> Where do these numbers come from? What will happen on high-DPI screens or with
> lengthy translations? I suggest you measure the text size to determine window
> size.

Done.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:85: return message.wParam;
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> Doesn't this cause a warning? I would rather cast to bool explicitly here.

Done. Opted for message.wParam == 0 in case of yes. This is logically an exit
code, so 0 makes more sense for the positive outcome.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/UpdateInstal...
src/engine/UpdateInstallDialog.cpp:116: const int buttonHeight = 30;
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> Same issue as with the text, hardcoded size will be problematic if font size
> changes or translation is overly long.

Done.

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

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/Updater.cpp#...
src/engine/Updater.cpp:141: dictionary->Lookup("updater",
"install-error-title").c_str(), MB_OK);
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> Add MB_ICONEXCLAMATION flag?

Done.

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/engine.rc
File src/engine/engine.rc (right):

http://codereview.adblockplus.org/11304082/diff/10001/src/engine/engine.rc#ne...
src/engine/engine.rc:2: #include "Resource.h"
On 2013/08/01 09:58:24, Wladimir Palant wrote:
> You should remove Resource.h - none of the IDs defined there are used.

Done.

[Wladimir Palant, 2013-08-02 13:19:49]

LGTM

[Oleksandr, 2013-08-04 06:04:48]

LGTM