Issue 5797488346791936: Issue 1107 - Support notifications (Closed)
|
Created:
Jan. 19, 2015, 12:50 p.m. by sergei Modified:
Jan. 27, 2015, 10:36 a.m. Reviewers:
Felix Dahlke CC:
Wladimir Palant Visibility:
Public. |
Description# works on MSVS 2010
# the support for QuestionListener will be in another commit because it's quite controversial and it's possible to start to use notifications without it.
Patch Set 1 #
Total comments: 16
Patch Set 2 : move GetNotificationTexts into Notification class #Patch Set 3 : move MarkAsShown into Notification class and get rid of locale arg in Notification::GetTexts #
Total comments: 18
Patch Set 4 : remove local Notifications #
Total comments: 20
Patch Set 5 : clean #
Total comments: 1
Patch Set 6 : simplify the expression #Patch Set 7 : C++03 #
Total comments: 37
Patch Set 8 : fix according to the comments #
Total comments: 1
Patch Set 9 : fix comment #
MessagesTotal messages: 18
http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:247: const std::string& id) const; Why do we need this in the public API? Shouldn't notifications be created internally only? http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:261: * application locale) Do we need this parameter at all? When would it make sense to get notifications in a different language? http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:266: const std::string& locale = std::string()) const; Why isn't this a methods of the Notification class? Same with MarkAsShown(). http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:272: void AddNotification(const std::tr1::shared_ptr<Notification>& value); Why do we need an API to add "local" notifications? Notifications are supposed to come from the server only.
http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:247: const std::string& id) const; On 2015/01/21 13:48:28, Wladimir Palant wrote: > Why do we need this in the public API? Shouldn't notifications be created > internally only? It is for local notifications. @Felix http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:261: * application locale) On 2015/01/21 13:48:28, Wladimir Palant wrote: > Do we need this parameter at all? When would it make sense to get notifications > in a different language? 1. The javascript notifications API has this argument, so I decided to expose it as well. As I saw we support the following notification structures: { ..., title: "some string" } and { ..., title: { "en-US": "English", "de-DE": "Deutsch" } } 2. BTW, I would say the comment does not correspond to the actual implementation, because the default locale is not the application locale, it is "en-US", see https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/notification.js#l61 . But it's not the part of this project and change. @Felix http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:266: const std::string& locale = std::string()) const; On 2015/01/21 13:48:28, Wladimir Palant wrote: > Why isn't this a methods of the Notification class? Same with MarkAsShown(). 1. GetNotificationTexts is moved into Notification class. 2. MarkAsShown() accepts notification ID, which provides another level of flexibility. I guess it would be better to have it here. http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:272: void AddNotification(const std::tr1::shared_ptr<Notification>& value); On 2015/01/21 13:48:28, Wladimir Palant wrote: > Why do we need an API to add "local" notifications? Notifications are supposed > to come from the server only. I guess it is designed for the cases when we can detect something on the client side and notify the client about it. BTW, notifications come not only from the server, there is https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/antiadblockInit.js which can create local notifications. @Felix
http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:247: const std::string& id) const; On 2015/01/21 15:58:55, sergei wrote: > It is for local notifications. What kind of local notifications? If the client application wants to show a notification, why would it want to take the detour through libadblockplus for that? And if libadblockplus code wants to show a notification - why does this need a public API? Note that the anti-adblock notification comes from core code itself. If we ever add that code to libadblockplus, the JavaScript code will simply call the corresponding methods in the notification module - all that without leaving the JavaScript engine. http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:261: * application locale) On 2015/01/21 15:58:55, sergei wrote: > 1. The javascript notifications API has this argument, Yes, and it is already unused there. All the more reason to omit this parameter in libadblockplus - it provides a high-level API, exposing each and every weird feature isn't the point. > 2. BTW, I would say the comment does not correspond to the actual > implementation, because the default locale is not the application locale, it is > "en-US" en-US is the fallback locale, if no translations for the given locale is found. That's irrelevant here, the important line of code is https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/notification.js#.... http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:266: const std::string& locale = std::string()) const; On 2015/01/21 15:58:55, sergei wrote: > 2. MarkAsShown() accepts notification ID, which provides another level of > flexibility. What kind of flexibility? If we have a Notification object, why do we need to work with IDs?
Looks good all in all! Just got back to the discussions at this point. One thing I'm missing though: The notification feature should be optional for libadblockplus clients, just like the updater. What seemed like the most obvious option to me was to invoke a callback instead of relying on the clients calling `GetNextToShow` - happens three minutes after startup on other platforms. However, I'd also be fine with having an explicit function (`InitNotifications`?) for enabling the notification system and leaving it up to clients when to call `GetNextToShow`. That'd be more honest, in a way, since without a core change we can't really _stop_ checking for notifications once we've initialised the module. http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:261: * application locale) On 2015/01/21 19:23:18, Wladimir Palant wrote: > On 2015/01/21 15:58:55, sergei wrote: > > 1. The javascript notifications API has this argument, > > Yes, and it is already unused there. All the more reason to omit this parameter > in libadblockplus - it provides a high-level API, exposing each and every weird > feature isn't the point. > > > 2. BTW, I would say the comment does not correspond to the actual > > implementation, because the default locale is not the application locale, it > is > > "en-US" > > en-US is the fallback locale, if no translations for the given locale is found. > That's irrelevant here, the important line of code is > https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/notification.js#.... Agreed. I actually think we should remove it from the core code, too: https://issues.adblockplus.org/ticket/1846 http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:266: const std::string& locale = std::string()) const; On 2015/01/21 19:23:18, Wladimir Palant wrote: > On 2015/01/21 15:58:55, sergei wrote: > > 2. MarkAsShown() accepts notification ID, which provides another level of > > flexibility. > > What kind of flexibility? If we have a Notification object, why do we need to > work with IDs? Agreed - there is no notification object in that sense in the core code, so we have to work with IDs. Here we don't have to. http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:272: void AddNotification(const std::tr1::shared_ptr<Notification>& value); On 2015/01/21 15:58:55, sergei wrote: > On 2015/01/21 13:48:28, Wladimir Palant wrote: > > Why do we need an API to add "local" notifications? Notifications are supposed > > to come from the server only. > > I guess it is designed for the cases when we can detect something on the client > side and notify the client about it. BTW, notifications come not only from the > server, there is > https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/antiadblockInit.js > which can create local notifications. > @Felix You're right Sergei, the question notification is currently not coming from the server. But as Wladimir said, all notifications are _supposed_ to come from the server, the anti adblock notification being added locally is a hack. Once we've made the notification system more powerful, that too, will be defined on the server. Any local considerations for showing a notification will be added to the notification mechanism as a feature, the core code will handle that.
Reviewed some parts of this already, but the biggest chunks not yet. But from what I've seen they're using C++11 stuff. libadblockplus is currently C++03 code, this is not the right time to change that, please stick to C++03. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:30: NOTIFICATION_TYPE_INFORMATION = 0, Why explicitly assign the values here? We typically don't do that with enums. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:31: NOTIFICATION_TYPE_QUESTION = 1, No column aligning please :D http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:35: struct NotificationTexts It's a single one, so "NotificationText"? http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:45: struct PrivateCtrArg{}; I still find this pattern highly obscure. We've got a discussion on whether to use it in libadblockplus elsewhere, let's please keep that there. If we are to use it, we should use it consistently.
http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/FilterEngine.h:247: const std::string& id) const; This method still needs to be removed, along with AddNotification and RemoveNotification. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/JsEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/JsEngine.h:151: JsValuePtr NewArray(); This change (and method Push) should be unnecessary once all the unnecessary APIs have been removed. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:52: std::string GetId() const; I think this method should be removed. The ID is an irrelevant implementation detail. The client application has the Notification object to refer to notifications. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:58: NotificationTexts GetTexts(); An explicit method here shouldn't be necessary. The texts should rather be retrieved when the notification is created and stored in protected properties. There should then be public getters for these properties - the NotificationTexts structure shouldn't be required. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:62: void AddUrlFilter(const std::string& value); The four methods above should be removed - as discussed, notifications should be read-only. The client application should never create its own notifications. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/test... File test/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/test... test/Notification.cpp:100: auto notification = filterEngine->CreateNotification(NotificationType::NOTIFICATION_TYPE_CRITICAL, "testId"); Instead of defining this API for tests only, please use JSEngine::Evaluate(), along these lines: jsEngine->Evaluate(" (function() { var Notification = require('notification').Notification; var TYPE = require('notification').TYPE; Notification.addNotification({ id: 'testId', type: 'critical' }); })();");
http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:261: * application locale) On 2015/01/22 03:31:08, Felix H. Dahlke wrote: > On 2015/01/21 19:23:18, Wladimir Palant wrote: > > On 2015/01/21 15:58:55, sergei wrote: > > > 1. The javascript notifications API has this argument, > > > > Yes, and it is already unused there. All the more reason to omit this > parameter > > in libadblockplus - it provides a high-level API, exposing each and every > weird > > feature isn't the point. > > > > > 2. BTW, I would say the comment does not correspond to the actual > > > implementation, because the default locale is not the application locale, it > > is > > > "en-US" > > > > en-US is the fallback locale, if no translations for the given locale is > found. > > That's irrelevant here, the important line of code is > > > https://hg.adblockplus.org/adblockplus/file/ec627ace494e/lib/notification.js#.... > > Agreed. I actually think we should remove it from the core code, too: > https://issues.adblockplus.org/ticket/1846 I've removed locale for notification texts. http://codereview.adblockplus.org/5797488346791936/diff/5629499534213120/incl... include/AdblockPlus/FilterEngine.h:266: const std::string& locale = std::string()) const; On 2015/01/22 03:31:08, Felix H. Dahlke wrote: > On 2015/01/21 19:23:18, Wladimir Palant wrote: > > On 2015/01/21 15:58:55, sergei wrote: > > > 2. MarkAsShown() accepts notification ID, which provides another level of > > > flexibility. > > > > What kind of flexibility? If we have a Notification object, why do we need to > > work with IDs? > > Agreed - there is no notification object in that sense in the core code, so we > have to work with IDs. Here we don't have to. MarkAsShown is moved into Notification class. Just wonder, what is the reason that we cannot carry notification object but can carry its ID in the core? The flexibility is that carrying string is very simple while to preserve the notification object is additional work for the clients of libadblockplus. I guess that hosting libadblockplus in another process would not be a rare case, so the client of libadblockplus receives the notification object, serializes it and sends to the main application. Now this notification object could be removed if we provide the API which works with the ID, but without such API the client has to remember it somewhere. We cannot mark it as shown immediately because most likely the main application has already some notification mechanism with some queue, so it's not guaranteed that the user will see it and we should mark the notification as shown only after the actual showing. I guess it is the reason to have the method MarkAsShown in general and the notification is not removed from the pending notifications in the core. After showing of the notification the main application sends the notification id to the libadblockplus hoster, having this ID the hoster could easily mark it as shown. In addition, although right now there are no now notifications and as I understand they won't be often, the developer of the client of libadblockplus will definitely ask himself about "leaking" of the preserved notifications, at least for the case when the main app looses the notification ID (because of restart for example). If we switch from the polling approach the hoster of libadblockplus will discover that now in addition he has to synchronize the access to the collection of stored notifications. Later this poor developer can discover that he has to care about the destruction order, because JsEngine is referenced by the stored notification (see #1849). So, it complicates the hoster of libabdlockplus. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:30: NOTIFICATION_TYPE_INFORMATION = 0, On 2015/01/22 10:25:25, Felix H. Dahlke wrote: > Why explicitly assign the values here? We typically don't do that with enums. fixed http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:35: struct NotificationTexts On 2015/01/22 10:25:25, Felix H. Dahlke wrote: > It's a single one, so "NotificationText"? Actually there were two fields and the plural form is used in javascript (getLocalizedTexts), anyway it's removed now from here. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:45: struct PrivateCtrArg{}; On 2015/01/22 10:25:25, Felix H. Dahlke wrote: > I still find this pattern highly obscure. We've got a discussion on whether to > use it in libadblockplus elsewhere, let's please keep that there. If we are to > use it, we should use it consistently. JIC, if the constructor is not public then std::make_shared cannot get access to it. http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:52: std::string GetId() const; On 2015/01/22 10:47:20, Wladimir Palant wrote: > I think this method should be removed. The ID is an irrelevant implementation > detail. The client application has the Notification object to refer to > notifications. removed http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:58: NotificationTexts GetTexts(); On 2015/01/22 10:47:20, Wladimir Palant wrote: > An explicit method here shouldn't be necessary. The texts should rather be > retrieved when the notification is created and stored in protected properties. > There should then be public getters for these properties - the NotificationTexts > structure shouldn't be required. fixed http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:62: void AddUrlFilter(const std::string& value); On 2015/01/22 10:47:20, Wladimir Palant wrote: > The four methods above should be removed - as discussed, notifications should be > read-only. The client application should never create its own notifications. fixed http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/incl... include/AdblockPlus/Notification.h:47: const std::string& GetMessageString() const; It's not GetMessage because it's not possible to call it when windows.h is included in the same cpp file due to #define GetMessage GetMessageW in windows.h.
On 2015/01/22 03:31:08, Felix H. Dahlke wrote: > One thing I'm missing though: The notification feature should be optional for > libadblockplus clients, just like the updater. > > What seemed like the most obvious option to me was to invoke a callback instead > of relying on the clients calling `GetNextToShow` - happens three minutes after > startup on other platforms. > > However, I'd also be fine with having an explicit function > (`InitNotifications`?) for enabling the notification system and leaving it up to > clients when to call `GetNextToShow`. > > That'd be more honest, in a way, since without a core change we can't really > _stop_ checking for notifications once we've initialised the module. Actually such things should come from the design of the core. But anyway, I find it very important to have notifications optional right now. So, what do you think about the following approach? We could have the method `void FilterEngine::SetOnNotification(const std::function<void(const NotificationPtr&)>& onNotification)` which ensures that notifications.js is loaded and polling timer is initialized. On the second call and further it can update the handler or stop the timer if `onNotification` is empty.
Just one comment for now :) http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:35: struct NotificationTexts On 2015/01/22 14:08:15, sergei wrote: > On 2015/01/22 10:25:25, Felix H. Dahlke wrote: > > It's a single one, so "NotificationText"? > > Actually there were two fields and the plural form is used in javascript > (getLocalizedTexts), anyway it's removed now from here. Argh you're right, of course it's two. Should really be NotificationTexts then, sorry :P
http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/incl... include/AdblockPlus/Notification.h:30: NOTIFICATION_TYPE_INFORMATION, NOTIFICATION_TYPE_QUESTION, NOTIFICATION_TYPE_CRITICAL Nit: listing these one per line would be better, this line is too long right now. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/JsEngine.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/JsEngine.cpp:150: return JsValuePtr(new JsValue(shared_from_this(), v8::Array::New())); That method is unused now. You might argue that it should be there for sake of completeness (note that I generally object to introducing unused code), but it definitely doesn't belong into this patch. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/JsValue.cpp:191: jsArray->Set(jsArray->Length(), value->UnwrapValue()); That method is unused now. You might argue that it should be there for sake of completeness (I doubt that as we have a way to call methods already), but it definitely doesn't belong into this patch. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:52: std::string NotificationTypeToString(NotificationType value) This function seems unused. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:96: params.push_back(jsEngine->NewValue(jsId ? jsId->AsString() : "")); Does it even make sense to call API.markNotificationAsShown() with a an empty string as parameter? http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:112: if (!func) Here and elsewhere, this kind of check is pointless. JSEngine::Evaluate() will never return an empty pointer. It could throw an exception (execution failed) or return a JSValue, nothing beyond that. What you really need to check is func->IsFunction() but func->Call() will already do that. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:119: if (!jsTexts) Here again a pointless check - JSValue::Call() will also either throw an exception or return a JSValue instance (which can be undefined, e.g. if the function didn't return a value). There is no scenario where it would return an empty pointer. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:125: if (jsTitle && jsTitle->IsString()) Checking whether jsTitle is true is pointless (same in other places where GetProperty() is used) - JSValue::GetProperty() will always return *something*, jsTitle->IsUndefined() will simply be true if the property doesn't exist. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:134: return notification; Shouldn't all this code be inside the constructor? What's the point of the factory function? Sure, a constructor cannot return an empty pointer, it can only throw an exception - but this function will throw exceptions as well, e.g. if there is no "API" variable in the JS context.
http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/incl... include/AdblockPlus/Notification.h:30: NOTIFICATION_TYPE_INFORMATION, NOTIFICATION_TYPE_QUESTION, NOTIFICATION_TYPE_CRITICAL On 2015/01/22 15:19:51, Wladimir Palant wrote: > Nit: listing these one per line would be better, this line is too long right > now. fixed http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/JsEngine.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/JsEngine.cpp:150: return JsValuePtr(new JsValue(shared_from_this(), v8::Array::New())); On 2015/01/22 15:19:51, Wladimir Palant wrote: > That method is unused now. You might argue that it should be there for sake of > completeness (note that I generally object to introducing unused code), but it > definitely doesn't belong into this patch. Sorry, forgot about it. Removed. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/JsValue.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/JsValue.cpp:191: jsArray->Set(jsArray->Length(), value->UnwrapValue()); On 2015/01/22 15:19:51, Wladimir Palant wrote: > That method is unused now. You might argue that it should be there for sake of > completeness (I doubt that as we have a way to call methods already), but it > definitely doesn't belong into this patch. Sure, removed http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:52: std::string NotificationTypeToString(NotificationType value) On 2015/01/22 15:19:51, Wladimir Palant wrote: > This function seems unused. removed http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:96: params.push_back(jsEngine->NewValue(jsId ? jsId->AsString() : "")); On 2015/01/22 15:19:51, Wladimir Palant wrote: > Does it even make sense to call API.markNotificationAsShown() with a an empty > string as parameter? Does not make sense. What do you think about passing `GetProperty("id")->AsString()`? http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:112: if (!func) On 2015/01/22 15:19:51, Wladimir Palant wrote: > Here and elsewhere, this kind of check is pointless. JSEngine::Evaluate() will > never return an empty pointer. It could throw an exception (execution failed) or > return a JSValue, nothing beyond that. What you really need to check is > func->IsFunction() but func->Call() will already do that. Clear, removed here and elsewhere. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:134: return notification; On 2015/01/22 15:19:51, Wladimir Palant wrote: > Shouldn't all this code be inside the constructor? What's the point of the > factory function? Sure, a constructor cannot return an empty pointer, it can > only throw an exception - but this function will throw exceptions as well, e.g. > if there is no "API" variable in the JS context. I would say the current variant is better for present. To call "API.getNotificationTexts" we need to pass the notification javascript object as argument, thus to put it into `JsValueList` which is a container of shared pointers of `JsValue`. - We cannot put `jsValue` constructor argument into JsValueList because its internal `V8ValueHolder<v8::Value> value` has been already transferred into `value` of the base class of `Notification` class. In addition see http://codereview.adblockplus.org/5598762307158016/ . - We cannot use `shared_from_this` in the constructor because internal `weak_ptr` of `enable_shared_from_this` is not initialized yet. - Even more, we cannot easily have `JsValuePtr` as a member of `Notification` class because in that case we don't have the access to the protected member `JsValue::jsEngine`. Actually we can create another class which inherits from from `JsValue` and which exposes `JsEngine` and store a pointer to this class as a member of `Notification` class, but I'm afraid that it is even more confusing. Although having `JsValue` as a private member of `Notification` class looks very attractive, just in case I would like to have an access to base JsValue.
I think Felix also mentioned C++11 syntax which we don't support yet. I'll leave the rest of this review to him, my concerns have been addressed. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:96: params.push_back(jsEngine->NewValue(jsId ? jsId->AsString() : "")); On 2015/01/22 16:15:11, sergei wrote: > Does not make sense. What do you think about passing > `GetProperty("id")->AsString()`? Yes, I think we should just pass in the value of the id property here. It will produce strange results for "unexpected" input but that's just "trash in, trash out" - no way around it. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:134: return notification; Ok, this makes sense then, at least given my knowledge of C++. Maybe fhd has a better idea. http://codereview.adblockplus.org/5797488346791936/diff/5703128158568448/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5703128158568448/src/... src/Notification.cpp:76: params.push_back(jsEngine->NewValue(GetProperty("id")->AsString())); jsEngine->NewValue(GetProperty("id")->AsString()) is a convoluted way of saying GetProperty("id"), right?
Alright, reviewed everything now! A bunch of comments, but I think we're pretty close, looks great all in all. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... File include/AdblockPlus/FilterEngine.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/FilterEngine.h:243: * @return Notification to be shown, or null if there is no any. Can you add backticks around null? (`null`). That way it shows up formatted as code. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/FilterEngine.h:245: std::tr1::shared_ptr<Notification> GetNextNotificationToShow( Why not use NotificationPtr here? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/Notification.h:39: struct PrivateCtrArg{}; As I said before - this is the wrong place to discuss whether to use this pattern, that discussion is elsewhere. For now let's keep things consistent. Assuming this is just for std::tr1::make_shared - can't we make that a friend here? Alternatively, we could just use new - I know it's better to use make_shared, but we're using new all over the place anyway, and the drawbacks seem a bit academic. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/Notification.h:45: * Localizes the texts of the supplied notification. Shouldn't this comment say that it returns just the title? Also, GetMessage should get one. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/Notification.h:46: * @return the translated texts. We consistently start with an upper case letter after @return in libadblockplus, we should do that here, too. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/Notification.h:49: const std::string& GetMessageString() const; Why not just GetMessage()? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/lib/... lib/prefs.js:44: shown: [], Wouldn't it work all the same if we initialised notificationdata to just `{}` here? That's what adblockpluschrome does. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/FilterEngine.cpp:260: std::tr1::shared_ptr<Notification> FilterEngine::GetNextNotificationToShow(const std::string& url /*= std::string()*/) Not a fan of that comment - this kind of stuff tends to get out of sync with reality, i.e. if the default value changes in the header. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:38: const NotificationTypes g_notificationTypes = InitNotificationTypes(); No hungarian notation please :) https://adblockplus.org/en/coding-style#general http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:50: NotificationTypes::const_iterator ci_notificationType = std::find_if( Hungarian again :) http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:87: NotificationPtr Notification::JsValueToNotification(const JsValuePtr& jsValue) Why not simply do this in the constructor, as Filter, Subscription etc. do it? Then we could also call std::tr1::make_shared with a public constructor in FilterEngine.cpp and the PrivateCtr thing shouldn't be necessary to begin with. Or am I missing something? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:89: if(!jsValue || !jsValue->IsObject()) Single space before ( please. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... File test/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:22: /* This define enables NotificationMockWebRequestTest but to run it Shouldn't this comment be right above the define in question? Anyway, I think this is not a bad solution for now, but I also think we should create a follow-up issue for improving this. Only achieving full test coverage when manually editing notification.js is not ideal. Maybe we should show a pragma message if this isn't defined, too? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:27: //*/ Seems like this can be removed :) http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:40: jsEngine->SetFileSystem(FileSystemPtr(new LazyFileSystem())); Why use make_shared below for this but not here? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:41: jsEngine->SetWebRequest(std::tr1::make_shared<LazyWebRequest>()); Why use make_shared here? Strictly speaking, this expects a WebRequestPtr, which may or may not be an std::tr1::shared_ptr. But I get the impression we should probably have make_shared typedefs corresponding to all these shared_ptr typedefs we have, huh? Stuff for a follow-up issue. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:46: void AddNotificaiton(const std::string& notification) Should be "AddNotification", huh? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:58: std::string m_responseText; Hungarian again! :D http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:83: void SetUp() override override is C++11, isn't it? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:114: std::this_thread::sleep_for(std::chrono::seconds(5)); // it's a hack Both std::thread and std::chrono are C++11. Can't we just use AdblockPlus::Sleep() like all the other tests? http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:118: EXPECT_EQ(nullptr, filterEngine->GetNextNotificationToShow().get()); nullptr is C++11, too. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:140: TEST_F(NotificationTest, FilterByUrl1) Don't see any FitlerByUrl2 :) http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:167: EXPECT_NE(nullptr, notification); Shouldn't you compare to notification.get() here like above?
http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/incl... include/AdblockPlus/Notification.h:45: * Localizes the texts of the supplied notification. On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Shouldn't this comment say that it returns just the title? Also, GetMessage > should get one. Sorry, fixed and more comments are added. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/lib/... File lib/prefs.js (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/lib/... lib/prefs.js:44: shown: [], On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Wouldn't it work all the same if we initialised notificationdata to just `{}` > here? That's what adblockpluschrome does. May be some previous test covered the case when it's not initialized. Now it works without `shown`. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/FilterEngine.cpp:260: std::tr1::shared_ptr<Notification> FilterEngine::GetNextNotificationToShow(const std::string& url /*= std::string()*/) On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Not a fan of that comment - this kind of stuff tends to get out of sync with > reality, i.e. if the default value changes in the header. removed http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:38: const NotificationTypes g_notificationTypes = InitNotificationTypes(); On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > No hungarian notation please :) https://adblockplus.org/en/coding-style#general fixed, although it's not hungarian notation http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:50: NotificationTypes::const_iterator ci_notificationType = std::find_if( On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Hungarian again :) fixed http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/src/... src/Notification.cpp:89: if(!jsValue || !jsValue->IsObject()) On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Single space before ( please. fixed http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... File test/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:22: /* This define enables NotificationMockWebRequestTest but to run it On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Shouldn't this comment be right above the define in question? > > Anyway, I think this is not a bad solution for now, but I also think we should > create a follow-up issue for improving this. Only achieving full test coverage > when manually editing notification.js is not ideal. Maybe we should show a > pragma message if this isn't defined, too? I guess it's better to have it at the beginning of the file than somewhere in the code. I would like to leave it as is and wait for the change in the core. I guess till that moment we will switch to some decent compiler and it will be unconditional. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:27: //*/ On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Seems like this can be removed :) Changed. It's a part of comment toggling. It also included <chrono> which is not available in msvs2010. commented: /* some code some code some code //*/ uncommented: //* some code some code some code //*/ JIC, it's also possible to have "branching" /* case A /*/ case B //*/ http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:40: jsEngine->SetFileSystem(FileSystemPtr(new LazyFileSystem())); On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Why use make_shared below for this but not here? I guess, because I've copied it and during the testing I changed the next line manually. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:41: jsEngine->SetWebRequest(std::tr1::make_shared<LazyWebRequest>()); On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Why use make_shared here? Strictly speaking, this expects a WebRequestPtr, which > may or may not be an std::tr1::shared_ptr. > > But I get the impression we should probably have make_shared typedefs > corresponding to all these shared_ptr typedefs we have, huh? Stuff for a > follow-up issue. I don't think it makes sense to have typedef for make_shared. http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:46: void AddNotificaiton(const std::string& notification) On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Should be "AddNotification", huh? thanks ) http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:114: std::this_thread::sleep_for(std::chrono::seconds(5)); // it's a hack On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Both std::thread and std::chrono are C++11. Can't we just use > AdblockPlus::Sleep() like all the other tests? fixed http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:118: EXPECT_EQ(nullptr, filterEngine->GetNextNotificationToShow().get()); On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > nullptr is C++11, too. fixed http://codereview.adblockplus.org/5797488346791936/diff/5743868070854656/test... test/Notification.cpp:167: EXPECT_NE(nullptr, notification); On 2015/01/23 13:25:55, Felix H. Dahlke wrote: > Shouldn't you compare to notification.get() here like above? no, if we use nullptr.
LGTM! Feel free to address the comment nit, but I don't think we need a new patch set for this. http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... File src/Notification.cpp (right): http://codereview.adblockplus.org/5797488346791936/diff/5641332169113600/src/... src/Notification.cpp:134: return notification; On 2015/01/22 19:16:18, Wladimir Palant wrote: > Ok, this makes sense then, at least given my knowledge of C++. Maybe fhd has a > better idea. I see... Don't think we have the time to really dive into this right now, but I think we should create a follow-up for this. Seems like a hack to me. http://codereview.adblockplus.org/5797488346791936/diff/5651874166341632/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5651874166341632/incl... include/AdblockPlus/Notification.h:71: * Marks this notification as shown. It is valid for question Maybe "only relevant" instead of "valid".
http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... File include/AdblockPlus/Notification.h (right): http://codereview.adblockplus.org/5797488346791936/diff/5631943370604544/incl... include/AdblockPlus/Notification.h:58: NotificationTexts GetTexts(); On 2015/01/22 14:08:15, sergei wrote: > On 2015/01/22 10:47:20, Wladimir Palant wrote: > > An explicit method here shouldn't be necessary. The texts should rather be > > retrieved when the notification is created and stored in protected properties. > > There should then be public getters for these properties - the > NotificationTexts > > structure shouldn't be required. > > fixed Missed this discussion before. I actually disagree, I don't see a real benefit to how it was implemented now, I preferred how it was before. Reasons: 1. We can actually use the constructor to fully construct the Notification object. 2. GetTexts().message is something Microsoft doesn't interfere with, as opposed to GetMessage(). 3. I really don't see how this could influence performance negatively. And even if it would, we could cache the texts in a local static. I think we should change this back again in a follow-up.
LGTM for the change. Please move Wladimir to CC and push. He was OK with an earlier iteration of this, and he's still sick. As for retrieving title/message on the fly to get rid of that static creator function: I still think we should do this, but let's do it in a separate review/commit. Can be a noissue thing I'd say.
On 2015/01/27 08:18:12, Felix H. Dahlke wrote: > LGTM for the change. > > Please move Wladimir to CC and push. He was OK with an earlier iteration of > this, and he's still sick. > > As for retrieving title/message on the fly to get rid of that static creator > function: I still think we should do this, but let's do it in a separate > review/commit. Can be a noissue thing I'd say. Pushed. Please find the version which reads notification properties here http://codereview.adblockplus.org/4904655779790848/. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
