Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(108)

Issue 6505394822184960: Issue 1109 - Support notifications (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by sergei
Modified:
4 years, 6 months ago
Reviewers:
Eric, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

# 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); # }

Patch Set 1 #

Total comments: 9

Patch Set 2 : rebase, fix nullptrs, use COM InternetExplorer and close NotificationWindow when link is clicked #

Total comments: 11

Patch Set 3 : improve ReplaceMulti and change loading of html template #

Patch Set 4 : remove non-used method #

Patch Set 5 : add border and shadow and remove title background #

Total comments: 7

Patch Set 6 : rebase and address comments #

Total comments: 3

Patch Set 7 : fix naming #

Total comments: 6

Patch Set 8 : address comments and a couple of additional fixes #

Patch Set 9 : fix obtaining of DPI value of content (NotificationWindow) of NotificationBorderWindow #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+2201 lines, -1253 lines) Patch
M adblockplus.gyp View 1 2 chunks +4 lines, -0 lines 0 comments Download
A html/templates/NotificationWindow.html View 1 2 3 4 5 1 chunk +64 lines, -0 lines 0 comments Download
M installer/src/msi/adblockplusie.wxs View 1 1 chunk +836 lines, -832 lines 0 comments Download
M src/engine/Main.cpp View 1 2 3 4 5 6 7 6 chunks +230 lines, -24 lines 9 comments Download
A src/engine/NotificationWindow.h View 1 2 3 4 5 6 7 1 chunk +305 lines, -0 lines 0 comments Download
A src/engine/NotificationWindow.cpp View 1 2 3 4 5 6 7 8 1 chunk +371 lines, -0 lines 2 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 2 chunks +26 lines, -58 lines 0 comments Download
M src/plugin/PluginFilter.cpp View 1 1 chunk +1 line, -44 lines 0 comments Download
A + src/shared/MsHTMLUtils.h View 1 2 3 4 5 6 7 1 chunk +37 lines, -32 lines 1 comment Download
A src/shared/MsHTMLUtils.cpp View 1 2 3 4 5 6 7 1 chunk +50 lines, -0 lines 0 comments Download
M src/shared/Utils.h View 1 1 chunk +81 lines, -80 lines 0 comments Download
M src/shared/Utils.cpp View 1 1 chunk +196 lines, -183 lines 0 comments Download

Messages

Total messages: 26
sergei
It's quite big, so if you think we should commit something separately, let me know, ...
4 years, 9 months ago (2015-05-11 10:06:20 UTC) #1
Eric
On 2015/05/11 10:06:20, sergei wrote: > It's quite big, so if you think we should ...
4 years, 9 months ago (2015-05-17 17:23:34 UTC) #2
Eric
http://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/NotificationWindow.html File src/engine/NotificationWindow.html (right): http://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/NotificationWindow.html#newcode1 src/engine/NotificationWindow.html:1: <!DOCTYPE html> This file should go in 'html/templates', alongside ...
4 years, 9 months ago (2015-05-17 17:23:47 UTC) #3
sergei
On 2015/05/17 17:23:34, Eric wrote: > It's definitely too big to review all at once. ...
4 years, 9 months ago (2015-05-18 09:41:19 UTC) #4
Oleksandr
https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/Main.cpp#newcode21 src/engine/Main.cpp:21: We have the same in PluginStdAfx.h. Maybe it makes ...
4 years, 8 months ago (2015-06-24 01:22:35 UTC) #5
sergei
Several comments as we discussed in IRC to put it into comments. https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/Main.cpp File src/engine/Main.cpp ...
4 years, 8 months ago (2015-06-24 17:31:36 UTC) #6
sergei
https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/NotificationWindow.h File src/engine/NotificationWindow.h (right): https://codereview.adblockplus.org/6505394822184960/diff/5629499534213120/src/engine/NotificationWindow.h#newcode25 src/engine/NotificationWindow.h:25: ATL::_U_MENUorID MenuOrID = 0U, LPVOID lpCreateParam = NULL) On ...
4 years, 8 months ago (2015-06-25 13:19:18 UTC) #7
Oleksandr
https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/Main.cpp#newcode519 src/engine/Main.cpp:519: void ApplicationMessageLoop(HINSTANCE hInstance) The whole idea behind this message ...
4 years, 8 months ago (2015-06-26 23:05:19 UTC) #8
Oleksandr
Also, about Eric's remark above about changing the security descriptors of the engine: > You're ...
4 years, 8 months ago (2015-06-26 23:30:45 UTC) #9
sergei
https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29321093/src/engine/Main.cpp#newcode519 src/engine/Main.cpp:519: void ApplicationMessageLoop(HINSTANCE hInstance) On 2015/06/26 23:05:18, Oleksandr wrote: > ...
4 years, 8 months ago (2015-06-29 11:10:50 UTC) #10
sergei
@Oleksandr In the latest patch set I've styled the window a bit though the ugly ...
4 years, 7 months ago (2015-07-03 13:08:54 UTC) #11
Oleksandr
Overall the effort it takes to create a border seems conspicuous. I wonder if it ...
4 years, 7 months ago (2015-07-22 09:45:36 UTC) #12
sergei
Rebased on https://codereview.adblockplus.org/29322740/. It turned out into a lot of changes. https://codereview.adblockplus.org/6505394822184960/diff/29321404/src/engine/Main.cpp File src/engine/Main.cpp (right): ...
4 years, 7 months ago (2015-07-23 14:17:58 UTC) #13
Oleksandr
Just a few nits. Looks fine otherwise now. https://codereview.adblockplus.org/6505394822184960/diff/29322747/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29322747/src/engine/Main.cpp#newcode61 src/engine/Main.cpp:61: TaskPosted ...
4 years, 7 months ago (2015-07-29 10:47:17 UTC) #14
sergei
Sorry, overlooked.
4 years, 7 months ago (2015-07-29 11:19:51 UTC) #15
Oleksandr
A few nits, remaining. LGTM otherwise. https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/Main.cpp#newcode63 src/engine/Main.cpp:63: public: Nit: A ...
4 years, 6 months ago (2015-08-10 09:13:35 UTC) #16
sergei
https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29322892/src/engine/Main.cpp#newcode63 src/engine/Main.cpp:63: public: On 2015/08/10 09:13:34, Oleksandr wrote: > Nit: A ...
4 years, 6 months ago (2015-08-17 13:12:54 UTC) #17
sergei
It seems we don't need to process WM_SIZE messages, however I left it just in ...
4 years, 6 months ago (2015-08-18 10:15:18 UTC) #18
Oleksandr
https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/NotificationWindow.cpp File src/engine/NotificationWindow.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/NotificationWindow.cpp#newcode304 src/engine/NotificationWindow.cpp:304: m_dpi = hdc.GetDeviceCaps(LOGPIXELSX); Is this still needed, seeing that ...
4 years, 6 months ago (2015-08-18 11:47:41 UTC) #19
sergei
https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/NotificationWindow.cpp File src/engine/NotificationWindow.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/NotificationWindow.cpp#newcode304 src/engine/NotificationWindow.cpp:304: m_dpi = hdc.GetDeviceCaps(LOGPIXELSX); On 2015/08/18 11:47:40, Oleksandr wrote: > ...
4 years, 6 months ago (2015-08-18 11:58:41 UTC) #20
Oleksandr
LGTM
4 years, 6 months ago (2015-08-18 12:27:35 UTC) #21
Eric
Not LGTM. The life cycle of the new objects is taking us backwards. https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/Main.cpp File ...
4 years, 6 months ago (2015-08-19 17:43:39 UTC) #22
Oleksandr
https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/Main.cpp File src/engine/Main.cpp (right): https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/Main.cpp#newcode20 src/engine/Main.cpp:20: processorArchitecture='*' publicKeyToken='6595b64144ccf1df' language='*'\"") On 2015/08/19 17:43:37, Eric wrote: > ...
4 years, 6 months ago (2015-08-20 13:12:14 UTC) #23
sergei
If something is important or even urgent please create an issue for it. https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/engine/Main.cpp File ...
4 years, 6 months ago (2015-08-20 14:45:46 UTC) #24
Eric
https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/shared/MsHTMLUtils.h File src/shared/MsHTMLUtils.h (right): https://codereview.adblockplus.org/6505394822184960/diff/29323889/src/shared/MsHTMLUtils.h#newcode23 src/shared/MsHTMLUtils.h:23: #include <atlbase.h> This statement breaks the build in Visual ...
4 years, 6 months ago (2015-08-20 17:57:47 UTC) #25
Eric
4 years, 6 months ago (2015-08-20 17:59:59 UTC) #26
Message was sent while issue was closed.
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5