Issue 11557015: Tooltip notification. Check for update fixes. (Closed)
|
Created:
Sept. 5, 2013, 10:43 p.m. by Oleksandr Modified:
Oct. 7, 2013, 10:03 p.m. Visibility:
Public. |
DescriptionTooltip notification. Check for update fixes.
Patch Set 1 #
Total comments: 27
Patch Set 2 : Addressing comments #
Total comments: 18
Patch Set 3 : More comments addressed #Patch Set 4 : More and more comments addressing #
Total comments: 3
Patch Set 5 : Evem more comments addressing #
Total comments: 5
Patch Set 6 : Nits addressed #MessagesTotal messages: 17
http://codereview.adblockplus.org/11557015/diff/1/adblockplus.gyp File adblockplus.gyp (right): http://codereview.adblockplus.org/11557015/diff/1/adblockplus.gyp#newcode102 adblockplus.gyp:102: 'defines': ['PRODUCT_ADBLOCKPLUS', 'ISOLATION_AWARE_ENABLED'], See "Adding Visual Style Support to an Extension, Plug-in, MMC Snap-in or a DLL That Is Brought into a Process" on the link here: http://msdn.microsoft.com/en-us/library/windows/desktop/bb773175(v=vs.85).asp...
http://codereview.adblockplus.org/11557015/diff/1/locales/en.ini File locales/en.ini (right): http://codereview.adblockplus.org/11557015/diff/1/locales/en.ini#newcode22 locales/en.ini:22: checking-for-updates-text=Adblock Plus is checking for a new version IMHO that should be (better formulation and a period at the end): Adblock Plus is looking for available updates. The title should be "Looking for updates" then. http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp#newcode53 src/engine/Main.cpp:53: checkingForUpdate = false; This needs to be set when the function is finished. As it is now this is a race condition - a new PROC_CHECK_FOR_UPDATES call might come in on a different thread after you set checkingForUpdate to false and the code below will send the response to the wrong thread. IMHO this function should now go like this: UINT message; if (updateAvailable) message = WM_DOWNLOADING_UPDATE else if (res.length() == 0) message = ...; else message = ...; if (callbackWindow) { SendMessage(callbackWindow, message, 0, 0); callbackWindow = 0; } This will also give you a single exit point where you can reset the checkingForUpdate flag. http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp#newcode63 src/engine/Main.cpp:63: Dictionary* dictionary = Dictionary::GetInstance(); The dictionary is no longer being used. http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp#newcode261 src/engine/Main.cpp:261: updateAvailable = false; Setting updateAvailable definitely belongs into the |if (!checkingForUpdate)| block. Not sure whether replacing the callbackWindow variable makes sense if the update check is triggered twice - the client simply shouldn't open two windows. Using static_cast<int32_t>() is certainly preferable here however. http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp#newcode264 src/engine/Main.cpp:264: checkingForUpdate = true; This is a race condition - another PROC_CHECK_FOR_UPDATES request can come in on a different thread after you checked checkingForUpdate but before you've set it to true. This will still result in two concurrent update checks. Access to checkingForUpdate needs to be protected with a lock, using "volatile" keyword won't do. http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:447: request << Communication::PROC_CHECK_FOR_UPDATES << (int32_t&)callbackWindow; Please use static_cast here. http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:220: commControls.dwICC = ICC_USEREX_CLASSES | ICC_STANDARD_CLASSES | ICC_BAR_CLASSES; Isn't ICC_BAR_CLASSES enough for a tooltip? http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:221: InitCommonControlsEx(&commControls); Given that common controls are used by the NotificationMessage class, that's where I would initialize them. E.g. use a static boolean variable commonControlsInitialized and run this code in the constructor if it is false. http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:1285: notifMessage.SetTextAndIcon(checkingText.c_str(), checkingTitle.c_str(), TTI_INFO); Assuming that NotificationMessage is our class, these methods should take const std::wstring& as paramers, not WCHAR*. Also, not quite understanding why we have to pass in checkingText twice. http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:1797: pClass->notifMessage.Hide(); Shouldn't the notification be only hidden if we get UIS_CLEAR in wParam? http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:1804: pClass->notifMessage.Move(rect.left + (rect.right - rect.left) / 2, rect.top + (rect.bottom - rect.top) / 2); Shouldn't we check whether the notification is actually visible before wasting time on adjusting its position? http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.h File src/plugin/PluginClass.h (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.h#ne... src/plugin/PluginClass.h:16: #include "NotificationMessage.h" This file isn't part of the patchset and I cannot see it in the repository either, did you forget to add it? http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.h#ne... src/plugin/PluginClass.h:146: NotificationMessage notifMessage; Nit: I don't like randomly abbreviated variable names. I think that we are all using editors with autocomplete functionality so spelling out notificationMessage isn't a big deal.
Nice, I had some trouble showing a tooltip if you recall, but I definitely prefer it. One question: The tooltips will also be shown during automatic update, right? I think we should show them only during manual update, no need to bother the user with this if he didn't actively decide to update IMO. http://codereview.adblockplus.org/11557015/diff/1/adblockplus.gyp File adblockplus.gyp (right): http://codereview.adblockplus.org/11557015/diff/1/adblockplus.gyp#newcode102 adblockplus.gyp:102: 'defines': ['PRODUCT_ADBLOCKPLUS', 'ISOLATION_AWARE_ENABLED'], On 2013/09/05 22:48:01, Oleksandr wrote: > See "Adding Visual Style Support to an Extension, Plug-in, MMC Snap-in or a DLL > That Is Brought into a Process" on the link here: > http://msdn.microsoft.com/en-us/library/windows/desktop/bb773175%28v=vs.85%29... Worth a comment IMO. http://codereview.adblockplus.org/11557015/diff/1/locales/en.ini File locales/en.ini (right): http://codereview.adblockplus.org/11557015/diff/1/locales/en.ini#newcode22 locales/en.ini:22: checking-for-updates-text=Adblock Plus is checking for a new version On 2013/09/11 13:07:06, Wladimir Palant wrote: > IMHO that should be (better formulation and a period at the end): > > Adblock Plus is looking for available updates. > > The title should be "Looking for updates" then. I'd get rid of the "available" part. http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp#newcode14 src/engine/Main.cpp:14: Super nit, but I think a single empty line between the includes and the namespace suffices. http://codereview.adblockplus.org/11557015/diff/1/src/engine/Main.cpp#newcode50 src/engine/Main.cpp:50: volatile bool checkingForUpdate = false; Why use the volatile keyword here? I doubt it's what we want. http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:19: As before, no need to add another line of whitespace here IMO. http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:221: InitCommonControlsEx(&commControls); On 2013/09/11 13:07:06, Wladimir Palant wrote: > Given that common controls are used by the NotificationMessage class, that's > where I would initialize them. E.g. use a static boolean variable > commonControlsInitialized and run this code in the constructor if it is false. I'd rather keep this where it is, the call is not specific to creating a tooltip.
On 2013/09/11 15:22:59, Felix H. Dahlke wrote: > One question: The tooltips will also be shown during automatic update, right? No. They are only being displayed as part of the ForceUpdateCheck() sequence, not for the automatic updates.
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:221: InitCommonControlsEx(&commControls); On 2013/09/11 15:22:59, Felix H. Dahlke wrote: > I'd rather keep this where it is, the call is not specific to creating a > tooltip. It is. Looking at the documentation, InitCommonControlsEx() calls are additive - which means that this function can be called multiple times, with different flags. The idea behind it is that different parts of the application call with, with the features they need. A single centralized InitCommonControlsEx() call is unnecessary.
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:221: InitCommonControlsEx(&commControls); On 2013/09/12 14:13:34, Wladimir Palant wrote: > On 2013/09/11 15:22:59, Felix H. Dahlke wrote: > > I'd rather keep this where it is, the call is not specific to creating a > > tooltip. > > It is. Looking at the documentation, InitCommonControlsEx() calls are additive - > which means that this function can be called multiple times, with different > flags. The idea behind it is that different parts of the application call with, > with the features they need. A single centralized InitCommonControlsEx() call is > unnecessary. Then let's put it in the NotificationMessage class. We wouldn't even have to bother with the static variable.
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:221: InitCommonControlsEx(&commControls); NotificationMessage sounds best for me as well then. (Sorry I forgot to include it in this patchset. Will be added in the next one) On 2013/09/12 14:23:49, Felix H. Dahlke wrote: > On 2013/09/12 14:13:34, Wladimir Palant wrote: > > On 2013/09/11 15:22:59, Felix H. Dahlke wrote: > > > I'd rather keep this where it is, the call is not specific to creating a > > > tooltip. > > > > It is. Looking at the documentation, InitCommonControlsEx() calls are additive > - > > which means that this function can be called multiple times, with different > > flags. The idea behind it is that different parts of the application call > with, > > with the features they need. A single centralized InitCommonControlsEx() call > is > > unnecessary. > > Then let's put it in the NotificationMessage class. We wouldn't even have to > bother with the static variable.
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:447: request << Communication::PROC_CHECK_FOR_UPDATES << (int32_t&)callbackWindow; On 2013/09/11 13:07:06, Wladimir Palant wrote: > Please use static_cast here. Since we are STRICT compliant: http://msdn.microsoft.com/en-us/library/windows/desktop/aa383731(v=vs.85).aspx) HWND isn't actually a pointer, but rather a struct: #define DECLARE_HANDLE(name) struct name##__{int unused;}; typedef struct name##__ *name So static_cast can't deal with it, and it's actually reinterpret_cast that this C-style cast resorts to. I can state reinterpret_cast explicitly, but I think current is a more portable solution.
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:447: request << Communication::PROC_CHECK_FOR_UPDATES << (int32_t&)callbackWindow; Felix seems to think that generally avoiding C-style casts is a good idea and I tend to agree.
http://codereview.adblockplus.org/11557015/diff/14001/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11557015/diff/14001/src/engine/Main.cpp#new... src/engine/Main.cpp:67: checkingForUpdate = false; I would normally protect every access to the variable with a lock. However, I cannot come up with a situation where not having a lock here would backfire. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:42: TOOLINFO ti; We should declare this as TOOLINFOW explicitly. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:43: ti.cbSize = sizeof(TOOLINFO); Indentation is off, convert tabs to spaces? http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:48: ti.lpszText = (LPWSTR)message.c_str(); I think that the conversion to LPWSTR is unnecessary here. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:51: LRESULT res = ::SendMessage(toolTipWindow, TTM_ADDTOOL, 0, (LPARAM) (LPTOOLINFO) &ti); The conversion to LPTOOLINFO is unnecessary - &ti is already a pointer to TOOLINFO. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:58: res = ::SendMessage(toolTipWindow, TTM_TRACKACTIVATE, TRUE, (LPARAM)(LPTOOLINFO) &ti); Same here, the conversion to LPTOOLINFO is unnecessary. Also, assigning the return value of SendMessage is unnecessary here and above if you don't actually use the return value. Finally, that function shouldn't duplicate the functionality from SetTextAndIcon() but rather call it. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:72: ::SendMessage(toolTipWindow, TTM_TRACKPOSITION, 0, (LPARAM)(LPTOOLINFO)MAKELONG(x, y)); The conversion to LPTOOLINFO is wrong here, that's not even a pointer we are sending. In fact, I don't think any conversion is needed whatsoever. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:79: ti.cbSize = sizeof(TOOLINFO); As above, this should be TOOLINFOW. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:84: ti.lpszText = (LPWSTR)text.c_str(); As above, the conversion to LPWSTR seems unnecessary. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:95: bool NotificationMessage::IsVisible() Nit: Empty line before this function. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:99: return IsWindowVisible(toolTipWindow); Given that we never hide the window but always destroy it - |return toolTipWindow != 0;| should do in this function. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... File src/plugin/NotificationMessage.h (right): http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.h:22: static void CommonControlsInitialize(); Nit: given that method names should be actions, InitializeCommonControls() would be the proper name. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/PluginClass.... src/plugin/PluginClass.cpp:599: notificationMessage.Hide(); I think IsVisible() check should happen inside the Hide() method.
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:447: request << Communication::PROC_CHECK_FOR_UPDATES << (int32_t&)callbackWindow; I agree with that as well, but in this case it is already a worst case scenario (ie we are using reinterpret_cast anyway). So there is no harm in using the C-style cast, and it increases readability, imho. On 2013/09/16 14:37:18, Wladimir Palant wrote: > Felix seems to think that generally avoiding C-style casts is a good idea and I > tend to agree. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:48: ti.lpszText = (LPWSTR)message.c_str(); On 2013/09/19 08:59:16, Wladimir Palant wrote: > I think that the conversion to LPWSTR is unnecessary here. We need to remove const here. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:99: return IsWindowVisible(toolTipWindow); On 2013/09/19 08:59:16, Wladimir Palant wrote: > Given that we never hide the window but always destroy it - |return > toolTipWindow != 0;| should do in this function. The window may become hidden without us hiding it. (As a result of some user interaction with top level windows, for example)
http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11557015/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:447: request << Communication::PROC_CHECK_FOR_UPDATES << (int32_t&)callbackWindow; On 2013/09/25 10:03:14, Oleksandr wrote: > So there is no harm in using the C-style cast, I disagree. Currently there are lots of C-style casts in the codebase and there is no way of recognizing whether they have been used mindlessly or because reinterpret_cast would need to be used anyway. The only way to change this is using reinterpret_cast when you mean reinterpret_cast. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:48: ti.lpszText = (LPWSTR)message.c_str(); On 2013/09/25 10:03:14, Oleksandr wrote: > We need to remove const here. Please use const_cast to indicate that. It's a pity that the Windows API is badly designed with respect to const-ness, this is the best we can do I think. http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:99: return IsWindowVisible(toolTipWindow); On 2013/09/25 10:03:14, Oleksandr wrote: > The window may become hidden without us hiding it. (As a result of some user > interaction with top level windows, for example) I see. We could react to WM_WINDOWPOSCHANGED then to destroy the window if it was hidden. If we leave it as it is now we should make sure that we don't leak that window - will NotificationMessage::Hide() be called nevertheless?
http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/14001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:99: return IsWindowVisible(toolTipWindow); Did that. Also if we do miss the NotificationMessage::Hide() for any reason, it will be called next time the Window is meant to be created. I still think actually querying the window handle here is better then just checking if it is null. On 2013/09/25 10:27:14, Wladimir Palant wrote: > On 2013/09/25 10:03:14, Oleksandr wrote: > > The window may become hidden without us hiding it. (As a result of some user > > interaction with top level windows, for example) > > I see. We could react to WM_WINDOWPOSCHANGED then to destroy the window if it > was hidden. If we leave it as it is now we should make sure that we don't leak > that window - will NotificationMessage::Hide() be called nevertheless?
http://codereview.adblockplus.org/11557015/diff/41001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/41001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:12: NotificationMessage::NotificationMessage(HWND parent) You can merge the two constuctors by specifying 0 as default value for the parent parameter. http://codereview.adblockplus.org/11557015/diff/41001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:63: res = ::SendMessage(toolTipWindow, TTM_SETTITLE, icon, (LPARAM)title.c_str()); Not sure where my comment on the previous changeset went but IMHO the code duplication here needs to be removed by calling SetTextAndIcon(). http://codereview.adblockplus.org/11557015/diff/41001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:71: if (IsVisible()) This seems to be the wrong check to me - we want to check toolTipWindow instead. We should destroy it even if it isn't visible. You probably do it like this for the sake of the return value but that return value isn't useful and can be dropped IMHO.
LGTM with the nit below fixed. http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:57: SetTextAndIcon(message.c_str(), title.c_str(), icon); Calling c_str() is unnecessary here. Also, this isn't quite what I meant - but seeing that the messages aren't really identical in the two cases, not setting ti.flags and ti.rect in SetTextAndIcon() should do to reduce code duplication.
A bunch of nits from me, otherwise LGTM. Regarding casts: C-style casts are a bit weird and I personally prefer to avoid them. They basically try all the more specific C++ style casts (except dynamic_cast) in turn until something works. The worst case is always both const_cast and reinterpret_cast. In places where we have to do that, I don't mind C-style casts for brevity. Otherwise we should avoid them IMO. http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... File src/plugin/NotificationMessage.cpp (right): http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:8: parentWindow = parent; Would rather do this in the initialiser list. http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:42: TOOLINFOW ti; Would rather have all members set to zeroes, just in case we don't set them all below. We typically do that, either by using ZeroMemory/memset or by default initialising (TOOLINFOW ti = {}). I prefer the latter. http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:48: ti.lpszText = const_cast<LPWSTR>(message.c_str()); Can you get rid of this trailing whitespace? http://codereview.adblockplus.org/11557015/diff/45001/src/plugin/Notification... src/plugin/NotificationMessage.cpp:80: TOOLINFOW ti; As above, would prefer to default initialise this to avoid accidental garbage values for fields we don't set. 
|
