Issue 1109 - Support notifications

# Won't go into the nearest release.

add showing of NotificationWindow for notifications without url.

Installer:
- install NotificationWindow.html into html/templates

Engine:
- use Microsoft.Windows.Common-Controls of version 6.0.0.0. Otherwise there are some artefacts with drawing.
- add global `_AtlModule`, it's used by ATL window classes.
- add message loop in the main thread and move communication stuff into separate thread.
- add timer which waits 3 sec and checks whether there is some notification. It looks as bad-logic, but it is how it should be done now. There are plans to change it, see #2420.
- add NotificationWindow class and some helper classes and functions.

Client (BHO) and shared:
- start engine with Medium integrity level, because under Low integrity level we cannot open a link. Anyway we will need engine running under Medium integrity level if we want to inject something into IE frame (host of tabs). The question about raising the privileges is suppressed by registry key (see bho_registry_value.wxi).
- move `GetHtmlElementAttribute` from 'src/plugin/PluginFilter.cpp' into shared because now it's also used by engine.
- add `GetExeDir` which returns the directory of the current executable (similar to `GetDllDir`).

# for testing one can use the following prefs.json (It does not work after updating of adblockplus submodule)
# {"currentVersion":"1.4","notificationdata":{"lastCheck":1435237921229,"softExpiration":1435317952049,"hardExpiration":1435406930323,"data":{"notifications":[{"id":"myId","title":"myTitle","message":"myMessage <a>www</a> <a>abp</a>","links":["http://www.com","https://adblockplus.org"]}],"version":"201502031430"},"lastError":0,"downloadStatus":"synchronize_ok","shown":[]}}
# Use Fiddler, add into OnBeforeResponse handler
# if (oSession.uriContains("notification.json")) {
#   var respBody = '{"notifications": [{"id": "myId", "title": "Update Adblock Plus", "message": "A new version of Adblock Plus is ready to be installed. Do you want to install it now?", "links": []}],"version": "201502031430" }';
#   oSession.utilSetResponseBody(respBody);
# }

[sergei, 2015-05-11 10:06:20]

It's quite big, so if you think we should commit something separately, let me
know, I will be glad to do it.

There are quite a lot of questionable things, feel free to comment them but keep
in mind that proposed alternative should not complicate a lot.

[Eric, 2015-05-17 17:23:34]

On 2015/05/11 10:06:20, sergei wrote:
> It's quite big, so if you think we should commit something separately, let me
> know, I will be glad to do it.

It's definitely too big to review all at once. I'm not going to try to do a
detailed review. There are just too many new simultaneously moving parts. In
addition, there are some foundational problems with this, both in design choices
and requirements.

The design choice to put any kind of UI in the engine is just broken. I just
cannot agree with this approach. We already have code in the plugin for
displaying a first-run page. That code is a perfectly adequate way of displaying
notifications. Doing otherwise makes for a very large amount of problematic
code.

The use of Common Controls version 6.0.0.0 doesn't work. It's too high a minimum
bar. This version may not even be present under XP and Vista (and I think also
the early versions of 7). This isn't very hard to figure out; there's an MSDN
article on it.
 
https://msdn.microsoft.com/en-us/library/windows/desktop/hh298349(v=vs.85).aspx

You're also changing the integrity level of the engine. I'll have to defer to
Oleksandr on this one, because he worked out all the details, but I do recall
there are a rather large set of subtleties here to worry about. When I see large
chunks of code just gone there, I can't believe that's right.

Both the version and the integrity level are consequences of putting the UI in
the engine. If you don't do that, all these problems just go away.

-----

Here are some suggestions about how to break up the work.

We are already passing control from the engine to the BHO for UI in the case of
updates. It's not well-written at present. It uses a rather convoluted flow of
control to compensate for the lack of any kind of ability for the engine to call
back into the BHO. In my opinion that's a good first step--to enhance the
communication module to enable the engine to request services of the plugin. 

In parallel, we can generalize the code that displays the first-run page to
display other things. (See 'CPluginClass::FirstRunThread')

We're injecting our settings object into the first run page and the user
settings page. I would recommend injecting it also in the notification page.
Then augment the settings object to deliver notification data. Do the text
manipulation in Javascript on that page rather than with C++.

Since the timer facility is transitory, it should be broken out into its own
source files so that it can be removed easily when the time comes. In parallel
with this, the control flow that triggers the display of the notification page
should not be timer-specific.

[Eric, 2015-05-17 17:23:47]

http://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/...
File src/engine/NotificationWindow.html (right):

http://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/...
src/engine/NotificationWindow.html:1: <!DOCTYPE html>
This file should go in 'html/templates', alongside all the first-run page.

[sergei, 2015-05-18 09:41:19]

On 2015/05/17 17:23:34, Eric wrote:
> It's definitely too big to review all at once. I'm not going to try to do a
> detailed review. There are just too many new simultaneously moving parts. In
> addition, there are some foundational problems with this, both in design
choices
> and requirements.
What would you like to review separately?

> The design choice to put any kind of UI in the engine is just broken. I just
> cannot agree with this approach. We already have code in the plugin for
> displaying a first-run page. That code is a perfectly adequate way of
displaying
> notifications. Doing otherwise makes for a very large amount of problematic
> code.
Could you argument why it is broken and how do you think it should be?
Apparently notification should not be a popup window as a first run page because
it's illogical to show popups when we are fighting against popup windows because
they are annoying. Showing some notification window from BHO does not seem to be
the proper way and would complicate the implementation a lot. There are many BHO
instances which are constantly birthing and dying but the lifetime of
notification window should not be limited by the lifetime of any instance of
BHO. So, the engine seems to be the most reasonable place for managing of
notification windows because we have a full control of the lifetime of the
engine and it's running until there is at least one instance of BHO, thus until
the user is using the browser.

> The use of Common Controls version 6.0.0.0 doesn't work. It's too high a
minimum
> bar. This version may not even be present under XP and Vista (and I think also
> the early versions of 7). This isn't very hard to figure out; there's an MSDN
> article on it.
>  
>
https://msdn.microsoft.com/en-us/library/windows/desktop/hh298349(v=vs.85).aspx
Firstly, for reference, the link in addition
http://www.geoffchappell.com/studies/windows/shell/comctl32/history/index.htm.
I don't see that it's stated that 6.0 is not available on windows XP or on newer
MS Windows versions. In addition to 6.0 there is an older version available for
compatibility reasons. So, common controls version 6.0 should work.

> Here are some suggestions about how to break up the work.
> 
> We are already passing control from the engine to the BHO for UI in the case
of
> updates. It's not well-written at present. It uses a rather convoluted flow of
> control to compensate for the lack of any kind of ability for the engine to
call
> back into the BHO. In my opinion that's a good first step--to enhance the
> communication module to enable the engine to request services of the plugin.
As said above it will complicate the things a lot because we will need to deal
with various kinds of race-conditions. It makes sense to show notification for
the specific page from the BHO, but it's not here yet.

> In parallel, we can generalize the code that displays the first-run page to
> display other things. (See 'CPluginClass::FirstRunThread')
I would say, beside we don't need it, it's too premature to generalize this page
for notifications.

> We're injecting our settings object into the first run page and the user
> settings page. I would recommend injecting it also in the notification page.
> Then augment the settings object to deliver notification data. Do the text
> manipulation in Javascript on that page rather than with C++.
I agree that we could inject some scripting object in the notification page but
now it will only add and complicate the code. Let's do it when we really need
something more than it is now.

> Since the timer facility is transitory, it should be broken out into its own
> source files so that it can be removed easily when the time comes.
We can still easily get rid of it. I would move it into its own file but because
of transitory of it I have kept it here, so we won't need to touch gyp file to
remove it which is, I guess, easier.

> In parallel with this, the control flow that triggers the display of the
notification page should not be timer-specific.
What do you mean? Current design of notifications in the core and the
requirement to be consistent with other platforms give no choice. If you mean
that for us it should be triggered from the core then I completely agree with it
and mentioned it many times and eventually there has been created an issue for
it https://issues.adblockplus.org/ticket/2366.

[Oleksandr, 2015-06-24 01:22:35]

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

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/Main.cpp:21: 
We have the same in PluginStdAfx.h. Maybe it makes sense to unite these and move
somewhere into common?

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/Main.cpp:602: ShellExecute(nullptr, L"open", url.c_str(), nullptr,
nullptr, SW_SHOWNORMAL);
This will open a default browser, not necessarily in IE. How about using
CoCreateInstance(CLSID_InternetExplorer ...). That way we'll be able to pass the
security info too, so there will not be the Medium Integrity requirement.

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/NotificationWindow.h:25: ATL::_U_MENUorID MenuOrID = 0U, LPVOID
lpCreateParam = NULL)
nullptr instead of NULL, for consistency? Also 2 lines above.

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
File src/engine/NotificationWindow.html (right):

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/NotificationWindow.html:3: <head>
Can't we just add <meta http-equiv="x-ua-compatible" content="IE=8"> here,
instead of all the registry stuff?
https://msdn.microsoft.com/en-us/library/jj676915(v=vs.85).aspx

[sergei, 2015-06-24 17:31:36]

Several comments as we discussed in IRC to put it into comments.

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

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/Main.cpp:21: 
On 2015/06/24 01:22:34, Oleksandr wrote:
> We have the same in PluginStdAfx.h. Maybe it makes sense to unite these and
move
> somewhere into common?

I don't think it's time to unite them.

Even more, I would prefer to use separate manifest files. Manifest file can
contain some properties which are applied when the application starts (like DPI
awareness), it seems useful to have all "manifest properties" in one place (in
the file) and these properties can be different between plugin and engine.

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/Main.cpp:602: ShellExecute(nullptr, L"open", url.c_str(), nullptr,
nullptr, SW_SHOWNORMAL);
On 2015/06/24 01:22:34, Oleksandr wrote:
> This will open a default browser, not necessarily in IE. How about using
> CoCreateInstance(CLSID_InternetExplorer ...). That way we'll be able to pass
the
> security info too, so there will not be the Medium Integrity requirement.

I was trying to launch internet explorer using CoCreateInstance from low IL and
to pass security info I tried CoInitializeSecurity and it does not work. Is it
what you meant? Without CoInitializeSecurity and when I pass nullptr for
security descriptor CoCreateInstance returns "Server execution failed"
(0x80080005). I'm tried to use the security descriptor which we use for pipe
(without SACL because in the documentation it's stated that SACL should be
nullptr and JIC with SACL as well as with and without DACL) and
CoInitializeSecurity returns invalid argument error. Do you know which arguments
we can use to achieve the goal and to satisfy CoInitializeSecurity ? If no, I
would propose to create an issue for it and make further investigations and fix
it in context of this issue.

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
File src/engine/NotificationWindow.html (right):

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/NotificationWindow.html:3: <head>
On 2015/06/24 01:22:34, Oleksandr wrote:
> Can't we just add <meta http-equiv="x-ua-compatible" content="IE=8"> here,
> instead of all the registry stuff?
> https://msdn.microsoft.com/en-us/library/jj676915(v=vs.85).aspx

Yes we can, in our case there is no difference.

[sergei, 2015-06-25 13:19:18]

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src...
src/engine/NotificationWindow.h:25: ATL::_U_MENUorID MenuOrID = 0U, LPVOID
lpCreateParam = NULL)
On 2015/06/24 01:22:34, Oleksandr wrote:
> nullptr instead of NULL, for consistency? Also 2 lines above.

Done.

[Oleksandr, 2015-06-26 23:05:19]

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

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/Main.cpp:519: void ApplicationMessageLoop(HINSTANCE hInstance)
The whole idea behind this message loop is just to handle the timer event for
notification. It seems to me that  using CreateTimerQueueTimer
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms682485(v=vs.85).aspx)
for this would be much more straightforward.

However, I think we don't need this timer at all now, do we? The #2420 has been
pushed already.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/Main.cpp:582: notificationWindow->Create(/*parent window*/nullptr);
How about:
notificationWindow->Create(/*parent window*/nullptr, 0,
ToUtf16String(notification->GetTitle()).c_str(), WS_SYSMENU,
WS_EX_PALETTEWINDOW);

That way we won't need to manage the title and the close button in the browser.
Also, as it is now the notification badly needs some sort of border, IMHO.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
File src/engine/NotificationWindow.cpp (right):

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.cpp:73: while ((i =
workingString.find(placeholder, i)) != std::wstring::npos)
How about putting everything in lowercase before this case? To handle stuff like
<A>, <!--body--> placeholders etc.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.cpp:94: m_htmlPage = [](const std::wstring&
filePath)->std::wstring
Do we really need a lambda function here? Seems to me something like this would
be better:

const std::wstring& filePath = htmlFileDir + L"NotificationWindow.html";
....
m_htmlPage.assign((std::istreambuf_iterator<wchar_t>(ifs)),
std::istreambuf_iterator<wchar_t>());

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.cpp:133: m_axIE.Create(m_hWnd,
DPIAware(CRect(CPoint(kIconSize, 0), CSize(kWindowWidth - kIconSize,
kWindowHeight))),
I think it would make sense to make the title to go outside of the browser.
Right now the title and the close button scroll with the message, if it's too
long. We know the title is just text, and also close button has only one default
purpose anyway.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.h:1: #pragma once
It is not clear yet if we will ever need such a rich functionality of the whole
rendering engine just to display notifications. Most likely we will only have a
text with link here. For that reusing NotificationMessage class would be better
(by exploiting TTF_PARSELINKS, for example. However we have no idea what our
notifications might look like, so I'm in favor for committing this JIC.

[Oleksandr, 2015-06-26 23:30:45]

Also, about Eric's remark above about changing the security descriptors of the
engine:
> You're also changing the integrity level of the engine. I'll have to defer to
> Oleksandr on this one, because he worked out all the details, but I do recall
> there are a rather large set of subtleties here to worry about. When I see
large
> chunks of code just gone there, I can't believe that's right.

The reason why we have a lot of code for the security descriptors is that we are
trying to play as safe as possible, and only elevate the integrity of our engine
when it is absolutely needed. This changset puts a requirement on the engine to
always run in medium integrity, hence the drastic cleanup of the code.

But, looking closer at the requirement of always running in medium integrity. It
is not ideal, of course, but it is the only way we can open IE tabs from the
engine now. Ideally I think we would be able to communicate back from the engine
to IE that we want to open a tab, but unfortunately we have only one way
communication right now. I doubt building a listener on the IE side would be in
scope of this review, seeing that it is already quite big. I think maybe it
would be a good idea to add support of notifications without link for now? We
can then add links and listener on IE side in other reviews.

[sergei, 2015-06-29 11:10:50]

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

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/Main.cpp:519: void ApplicationMessageLoop(HINSTANCE hInstance)
On 2015/06/26 23:05:18, Oleksandr wrote:
> The whole idea behind this message loop is just to handle the timer event for
> notification.
No, without message loop the notification window won't work.

> It seems to me that  using CreateTimerQueueTimer
>
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms682485(v=vs.85).aspx)
> for this would be much more straightforward.
Queue timers are not more straightforward and here they seem to be an overhead.

> However, I think we don't need this timer at all now, do we? The #2420 has
been
> pushed already.
#2420 is pushed but we cannot use it yet, we are waiting for
https://codereview.adblockplus.org/29317074/ and then we need to migrate to
ensure_dependencies.py.

As a summary we need message loop, we will get rid of the timer but not
immediately.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/Main.cpp:582: notificationWindow->Create(/*parent window*/nullptr);
On 2015/06/26 23:05:18, Oleksandr wrote:
> How about:
> notificationWindow->Create(/*parent window*/nullptr, 0,
> ToUtf16String(notification->GetTitle()).c_str(), WS_SYSMENU,
> WS_EX_PALETTEWINDOW);
> 
> That way we won't need to manage the title and the close button in the
browser.
Of course we can. The difference is that it looks ugly and unprofessional (see
how desktop notifications in Google Chrome look) , in my opinion and
inconsistent with notifications appearance on other platforms
(https://drive.google.com/drive/folders/0B63IPlLy5Xfvck9IWUZHdGZ5Nms). Having
the title and close button in html allows us to relatively quickly style it as
we need.
> Also, as it is now the notification badly needs some sort of border, IMHO.
I completely agree here, merely for reference I repeat that I think it should be
a shadow and not thick border.

To have a constructive discussion I would strongly request a design mockup from
Sven and then discuss it.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
File src/engine/NotificationWindow.cpp (right):

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.cpp:73: while ((i =
workingString.find(placeholder, i)) != std::wstring::npos)
On 2015/06/26 23:05:18, Oleksandr wrote:
> How about putting everything in lowercase before this case? To handle stuff
like
> <A>, <!--body--> placeholders etc.

Done.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.cpp:94: m_htmlPage = [](const std::wstring&
filePath)->std::wstring
On 2015/06/26 23:05:18, Oleksandr wrote:
> Do we really need a lambda function here? Seems to me something like this
would
> be better:
> 
> const std::wstring& filePath = htmlFileDir + L"NotificationWindow.html";
> ....
> m_htmlPage.assign((std::istreambuf_iterator<wchar_t>(ifs)),
> std::istreambuf_iterator<wchar_t>());

Sorry, forgot to clean it. During the developing I returned a simple template
instead of throwing of exception.
Done.

https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/...
src/engine/NotificationWindow.cpp:133: m_axIE.Create(m_hWnd,
DPIAware(CRect(CPoint(kIconSize, 0), CSize(kWindowWidth - kIconSize,
kWindowHeight))),
On 2015/06/26 23:05:18, Oleksandr wrote:
> I think it would make sense to make the title to go outside of the browser.
> Right now the title and the close button scroll with the message, if it's too
> long. We know the title is just text, and also close button has only one
default
> purpose anyway.

I don't mind but before doing it I would like to see mockups, see
https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/....
BTW, I think it's possible to make the title not scrollable via proper HTML and
CSS.

[sergei, 2015-07-03 13:08:54]

@Oleksandr
In the latest patch set I've styled the window a bit though the ugly scroll bar
is still there.
Notification window on a white background http://snag.gy/ROfXO.jpg.
If it causes a lot of questions and comments it's fine for me to make it as
another commit.

[Oleksandr, 2015-07-22 09:45:36]

Overall the effort it takes to create a border seems conspicuous. I wonder if it
would makes sense to just go for "flat" design with a border set in HTML, if
it's not too late?

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

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/Main.cpp:519: void ApplicationMessageLoop(HINSTANCE hInstance)
I still think using CreateTimerQueueTimer instead of a window listener would be
better, but I don't have a strong opinion. Also, since
https://codereview.adblockplus.org/29317074/ is closed we don't need a timer at
all now?

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/Main.cpp:659: 
NIT: Extra line here

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/NotificationWindow.h:100: logFont.lfHeight = -MulDiv(dpi,
logFont.lfHeight, 72);
I really don't see us going over the max int size with dpi * logFont.lfHeight
multiplication here. What's the reasoning behind using MulDiv instead of regular
multiplication? Would be more readable without MulDiv, IMHO. Also, I think "*
(-1)" is a more readable negation.

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/NotificationWindow.h:296: // m_content is used a holder of all
children and we need it to have a border.
NIT: "used AS a holder"

[sergei, 2015-07-23 14:17:58]

Rebased on https://codereview.adblockplus.org/29322740/.
It turned out into a lot of changes.

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

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/Main.cpp:519: void ApplicationMessageLoop(HINSTANCE hInstance)
On 2015/07/22 09:45:35, Oleksandr wrote:
> I still think using CreateTimerQueueTimer instead of a window listener would
be
> better, but I don't have a strong opinion. Also, since
> https://codereview.adblockplus.org/29317074/ is closed we don't need a timer
at
> all now?

What is a window listener? Anyway the timer is removed.

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/Main.cpp:659: 
On 2015/07/22 09:45:35, Oleksandr wrote:
> NIT: Extra line here

Fixed

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/...
src/engine/NotificationWindow.h:100: logFont.lfHeight = -MulDiv(dpi,
logFont.lfHeight, 72);
On 2015/07/22 09:45:36, Oleksandr wrote:
> I really don't see us going over the max int size with dpi * logFont.lfHeight
> multiplication here. What's the reasoning behind using MulDiv instead of
regular
> multiplication? Would be more readable without MulDiv, IMHO. Also, I think "*
> (-1)" is a more readable negation. 

We don't need CFont I forgot to remove it, I used it at the very first version
for title.

[Oleksandr, 2015-07-29 10:47:17]

Just a few nits. Looks fine otherwise now.

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

https://codereview.adblockplus.org/6505394822184960/diff/29322747/src/engine/...
src/engine/Main.cpp:61: TaskPosted = WM_USER + 1
Nit: TASK_POSTED. Our enums are almost always uppercase.

https://codereview.adblockplus.org/6505394822184960/diff/29322747/src/engine/...
src/engine/Main.cpp:651: void ABPAtlModule::dispatchTask(std::function<void()>&&
task)
Nit: How about "DispatchTask" instead? We usually have methods starting with an
upper case.

https://codereview.adblockplus.org/6505394822184960/diff/29322747/src/engine/...
src/engine/Main.cpp:660: void ABPAtlModule::processTasks()
Nit: ProcessTasks(). Upper case?

[sergei, 2015-07-29 11:19:51]

Sorry, overlooked.

[Oleksandr, 2015-08-10 09:13:35]

A few nits, remaining. LGTM otherwise.

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

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
src/engine/Main.cpp:63: public:
Nit: A new line before public and private in class definitions would be more
consistent with our coding style.

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
src/engine/Main.cpp:80: private:
Nit: Second "private" here is redundant

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
src/engine/NotificationWindow.h:1: #pragma once
Nit: We try to use GNU style guards in the new code. something like:
#ifndef NOTIFICATION_WINDOW_H
#define NOTIFICATION_WINDOW_H
..
#endif 

would be better

[sergei, 2015-08-17 13:12:54]

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

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
src/engine/Main.cpp:63: public:
On 2015/08/10 09:13:34, Oleksandr wrote:
> Nit: A new line before public and private in class definitions would be more
> consistent with our coding style.

Done.

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
src/engine/Main.cpp:80: private:
On 2015/08/10 09:13:34, Oleksandr wrote:
> Nit: Second "private" here is redundant

Done.

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
File src/engine/NotificationWindow.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/...
src/engine/NotificationWindow.h:1: #pragma once
On 2015/08/10 09:13:34, Oleksandr wrote:
> Nit: We try to use GNU style guards in the new code. something like:
> #ifndef NOTIFICATION_WINDOW_H
> #define NOTIFICATION_WINDOW_H
> ..
> #endif 
> 
> would be better

Done.

[sergei, 2015-08-18 10:15:18]

It seems we don't need to process WM_SIZE messages, however I left it just in
case.

The hosted browser is not properly scaled on IE9. Could not manage to get high
DPI on remote machine with IE8.

[Oleksandr, 2015-08-18 11:47:41]

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
File src/engine/NotificationWindow.cpp (right):

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/NotificationWindow.cpp:304: m_dpi = hdc.GetDeviceCaps(LOGPIXELSX);
Is this still needed, seeing that we now get dpi in OnCreate?

[sergei, 2015-08-18 11:58:41]

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
File src/engine/NotificationWindow.cpp (right):

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/NotificationWindow.cpp:304: m_dpi = hdc.GetDeviceCaps(LOGPIXELSX);
On 2015/08/18 11:47:40, Oleksandr wrote:
> Is this still needed, seeing that we now get dpi in OnCreate?

It's a little bit odd. `GetWindowCoordinates` is a member of
`NotificationBorderWindow` and is being called from
`NotificationBorderWindow::OnCreate`. In the latest patch we are obtaining DPI
for `NotificationWindow`.

[Oleksandr, 2015-08-18 12:27:35]

LGTM

[Eric, 2015-08-19 17:43:39]

Not LGTM. The life cycle of the new objects is taking us backwards.

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

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:20: processorArchitecture='*'
publicKeyToken='6595b64144ccf1df' language='*'\"")
On 2015/05/17 17:23:34, Eric wrote:
> The use of Common Controls version 6.0.0.0 doesn't work. It's too high a
> minimum bar. This version may not even be present under XP and Vista
> (and I think also the early versions of 7).

On 2015/05/18 09:41:19, sergei wrote:
> Firstly, for reference, the link in addition
> http://www.geoffchappell.com/studies/windows/shell/comctl32/history/index.htm.
> I don't see that it's stated that 6.0 is not available on windows XP or on
newer
> MS Windows versions. In addition to 6.0 there is an older version available
for
> compatibility reasons. So, common controls version 6.0 should work.

I did not say it wasn't available, I said it wasn't shipped by default. That's
not the same. It doesn't really matter that it's available from somewhere if
it's not present on disk when the BHO tries to load.

If you want to make it a requirement, you need to either (1) check for it in the
installer and fail out if it's not there, or (2) check for it in the installer
and install it if it's not present.

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:71: void RunMessageLoop() throw();
The only thing this message loop is doing is to pump the notification queue.
There are other ways of doing this that are less problematic.

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:88: } _AtlModule;
'_AtlModule' is defined statically, yet its proper functioning depends on
resources that this class doesn't control, notably the filter engine (containing
v8). The filter engine is initialized dynamically (in 'WinMain') but defined
statically as an 'auto_ptr'. This combination isn't good. There are three basic
approaches to fixing it.

1) Treat the filter engine as a global. This would mean changing the
initialization to static initialization. We'd have to deal with exceptions, but
we need to do that anyway, since the current dynamic initialization can throw a
C++ exception right through the entry point 'WinMain'. Then as long as static
variable 'filterEngine' is declared (and initialized) _before_ this object, it
will be constructed earlier and destroyed later, ensuring that v8 is present
whenever this object is present.

2) Treat the filter engine as a local. In this case, the culprit is the way that
the fact that member variables 'm_tasks' and 'm_notificationWindow' don't
contain references to the resources they need. The reference would be in the
form of a 'shared_ptr<AdblockPlus::FilterEngine>', which would mean needing to
change the declaration of 'filterEngine'. In the case of the tasks, that would
mean declaring the lambda expression with a capture of such a reference. In the
case of the notification window, the reference would be part of the constructor.
This all probably means making a factory for the filter engine.

3) Stop trying to overload this object to both pump the notification queue and
manage the life cycle of the notification window (which could be done in a task
object). There's no solid reason to place both of these responsibilities here.

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:474: // is already destroyed.
If it's possible for a dependency of a class to be destroyed while that class
still depends upon it, it means the class design is broken.

The very fact that this comment is necessary, much less the 'Finalize' function
itself, should have been evidence enough that this code needed attention.

[Oleksandr, 2015-08-20 13:12:14]

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

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:20: processorArchitecture='*'
publicKeyToken='6595b64144ccf1df' language='*'\"")
On 2015/08/19 17:43:37, Eric wrote:
> On 2015/05/17 17:23:34, Eric wrote:
> > The use of Common Controls version 6.0.0.0 doesn't work. It's too high a
> > minimum bar. This version may not even be present under XP and Vista
> > (and I think also the early versions of 7).
> 
> On 2015/05/18 09:41:19, sergei wrote:
> > Firstly, for reference, the link in addition
> >
http://www.geoffchappell.com/studies/windows/shell/comctl32/history/index.htm.
> > I don't see that it's stated that 6.0 is not available on windows XP or on
> newer
> > MS Windows versions. In addition to 6.0 there is an older version available
> for
> > compatibility reasons. So, common controls version 6.0 should work.
> 
> I did not say it wasn't available, I said it wasn't shipped by default. That's
> not the same. It doesn't really matter that it's available from somewhere if
> it's not present on disk when the BHO tries to load.
> 
> If you want to make it a requirement, you need to either (1) check for it in
the
> installer and fail out if it's not there, or (2) check for it in the installer
> and install it if it's not present.

Common Controls 6 are essentially Windows XP UI upgrades. They are by definition
included in Windows XP. A quick Google Search gave me this:
http://blogs.msdn.com/b/oldnewthing/archive/2008/01/29/7294949.aspx
http://www.geoffchappell.com/studies/windows/shell/comctl32/history/

On XP they are side-by-side with Common Controls 5. Which is why we need this
#pragma too.

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:88: } _AtlModule;
On 2015/08/19 17:43:37, Eric wrote:
> '_AtlModule' is defined statically, yet its proper functioning depends on
> resources that this class doesn't control, notably the filter engine
(containing
> v8). The filter engine is initialized dynamically (in 'WinMain') but defined
> statically as an 'auto_ptr'. This combination isn't good. There are three
basic
> approaches to fixing it.
> 
> 1) Treat the filter engine as a global. This would mean changing the
> initialization to static initialization. We'd have to deal with exceptions,
but
> we need to do that anyway, since the current dynamic initialization can throw
a
> C++ exception right through the entry point 'WinMain'. Then as long as static
> variable 'filterEngine' is declared (and initialized) _before_ this object, it
> will be constructed earlier and destroyed later, ensuring that v8 is present
> whenever this object is present.
> 
> 2) Treat the filter engine as a local. In this case, the culprit is the way
that
> the fact that member variables 'm_tasks' and 'm_notificationWindow' don't
> contain references to the resources they need. The reference would be in the
> form of a 'shared_ptr<AdblockPlus::FilterEngine>', which would mean needing to
> change the declaration of 'filterEngine'. In the case of the tasks, that would
> mean declaring the lambda expression with a capture of such a reference. In
the
> case of the notification window, the reference would be part of the
constructor.
> This all probably means making a factory for the filter engine.
> 
> 3) Stop trying to overload this object to both pump the notification queue and
> manage the life cycle of the notification window (which could be done in a
task
> object). There's no solid reason to place both of these responsibilities here.

It's not perfect right now, but good enough, IMHO. It isn't an actual bug, just
an architectural change. Since this code has already been pushed (sorry, we just
want to make a devbuild with it), I suggest just creating an issue for this, if
you think it is important.

[sergei, 2015-08-20 14:45:46]

If something is important or even urgent please create an issue for it.

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

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:71: void RunMessageLoop() throw();
On 2015/08/19 17:43:37, Eric wrote:
> The only thing this message loop is doing is to pump the notification queue.
What does the notification queue refers here?
Right now it's used to dispatch messages to notification window and for
synchronization between threads. I personally would like to use it also for
update stuff (window and make asynchronous downloading), for possible messages
from uninstaller (installer) custom action as well as we could utilize it for
libadblockplus to get rid of those free-running threads (it should normalize the
lifetime of objects and simplify the design) and may be for pipes.
> There are other ways of doing this that are less problematic.
What are the other ways and what is especially problematic here?

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:88: } _AtlModule;
On 2015/08/19 17:43:37, Eric wrote:
> '_AtlModule' is defined statically, yet its proper functioning depends on
> resources that this class doesn't control, notably the filter engine
(containing
> v8). The filter engine is initialized dynamically (in 'WinMain') but defined
> statically as an 'auto_ptr'. This combination isn't good. There are three
basic
> approaches to fixing it.
> 
> 1) Treat the filter engine as a global. This would mean changing the
> initialization to static initialization. We'd have to deal with exceptions,
but
> we need to do that anyway, since the current dynamic initialization can throw
a
> C++ exception right through the entry point 'WinMain'. Then as long as static
> variable 'filterEngine' is declared (and initialized) _before_ this object, it
> will be constructed earlier and destroyed later, ensuring that v8 is present
> whenever this object is present.
> 
> 2) Treat the filter engine as a local. In this case, the culprit is the way
that
> the fact that member variables 'm_tasks' and 'm_notificationWindow' don't
> contain references to the resources they need. The reference would be in the
> form of a 'shared_ptr<AdblockPlus::FilterEngine>', which would mean needing to
> change the declaration of 'filterEngine'. In the case of the tasks, that would
> mean declaring the lambda expression with a capture of such a reference. In
the
> case of the notification window, the reference would be part of the
constructor.
> This all probably means making a factory for the filter engine.
> 
> 3) Stop trying to overload this object to both pump the notification queue and
> manage the life cycle of the notification window (which could be done in a
task
> object). There's no solid reason to place both of these responsibilities here.

I faced with the troubles that it's static and yet another global variable as
well as with troubles related to uncontrolled _filterEngine and JsEngine and
decided to defer the refactoring so far, only for the sake of smaller amount of
the changes and of simplifying of this review.

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/...
src/engine/Main.cpp:474: // is already destroyed.
On 2015/08/19 17:43:38, Eric wrote:
> If it's possible for a dependency of a class to be destroyed while that class
> still depends upon it, it means the class design is broken.
> 
> The very fact that this comment is necessary, much less the 'Finalize'
function
> itself, should have been evidence enough that this code needed attention.
Yes, this code needs attention. I would like to note that, firstly, the design
of using `exit` and global variables is broken, secondly, the design of lifetime
of `AdblockPlus::JsEngine` and using of threads in libadblockplus is broken and
as the result we need this hack with `Finalize`.

[Eric, 2015-08-20 17:57:47]

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/shared/...
File src/shared/MsHTMLUtils.h (right):

https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/shared/...
src/shared/MsHTMLUtils.h:23: #include <atlbase.h>
This statement breaks the build in Visual Studio Express.

See https://issues.adblockplus.org/ticket/2940

[Eric, 2015-08-20 17:59:59]

I am not inclined to incur a discussion on this issue after it has been closed.
If we want to discuss it, I would like to see the change set reverted in the
repository and the issue reopened.