Bug 73194

Created attachment 116922 [details] Patch for notification support. V1.

Comment on attachment 116922 [details] Patch for notification support. V1. View in context: https://bugs.webkit.org/attachment.cgi?id=116922&action=review This is an informal review. > Source/WebKit/ChangeLog:3 > +2011-11-29 Robin Qiu <robin.qiu@torchmobile.com.cn> > + > + [BlackBerry] Add notification support for the BlackBerry port. As usual this should be the same as bug title. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:55 > +NotificationPresenterImpl::~NotificationPresenterImpl() > +{ > +} > + > + Extra blank line, please remove one. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:57 > +bool NotificationPresenterImpl::show(Notification* n) > +{ Should it ASSERT(n) here? > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:81 > +void NotificationPresenterImpl::cancel(Notification* n) > +{ Ditto. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:102 > +void NotificationPresenterImpl::onPermission(const std::string& domainStr, bool isAllowed) > +{ > + String domain = String::fromUTF8(domainStr.c_str()); Better ASSERT(!domainStr.empty()) before the converting? > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:137 > +void NotificationPresenterImpl::notificationClicked(const std::string& idStr) > +{ > + String id = String::fromUTF8(idStr.c_str()); Ditto > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:64 > + // Checks the current level of permission. > + virtual Permission checkPermission(WebCore::ScriptExecutionContext*); > + > + Extra blank line, please remove one.

Created attachment 117108 [details] Patch for notification support. V2. Modified according Leo's comments.

> > > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:64 > > + // Checks the current level of permission. > > + virtual Permission checkPermission(WebCore::ScriptExecutionContext*); > > + > > + > > Extra blank line, please remove one. The following 2 functions are inherited from another base class, so I added a extra blank line to separate.

Comment on attachment 117108 [details] Patch for notification support. V2. View in context: https://bugs.webkit.org/attachment.cgi?id=117108&action=review > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:23 > +#include "NotificationPresenterImpl.h" As per (2) of section #include Statements of <http://www.webkit.org/coding/coding-style.html>, move this line such that it's after #include "config.h". > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:27 > +#include "NotificationPresenterBlackBerry.h" This should be: #include <NotificationPresenterBlackBerry.h> > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:29 > +#include "StringHash.h" This should be: #include <wtf/text/StringHash.h> > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:37 > +NotificationPresenterImpl* NotificationPresenterImpl::m_instance = 0; Either this variable needs to be renamed to s_instance or we should define this variable in NotificationPresenterImpl::instance() using DEFINE_STATIC_LOCAL() since it's the only function that references this variable. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:69 > + // FIXME: load and display HTML content Nit: load => Load Also, comments should be full sentences that end in a period. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:72 > + // FIXME: strip to one line Nit: strip => Strip Also, comments should be full sentences that end in a period. I'm not entirely clear what this comment means. I take it that either notification->contents().title() or notification->contents().body() may contain some kind of line delimiter character(s) (e.g. new line characters) and that we want to remove such characters such that the message is ultimately displayed as one line of text. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:73 > + message = notification->contents().title() + ": " + notification->contents().body(); We should use makeString() (*) here: message = makeString(notification->contents().title(), ": ", notification->contents().body()); (*) Defined in JavaScriptCore/wtf/text/StringConcatenate.h: <http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/text/StringConcatenate.h?rev=98624#L621>. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:102 > +void NotificationPresenterImpl::onPermission(const std::string& domainStr, bool isAllowed) Nit: domainStr => domain (this matches the name of this parameter in the declaration of this function in NotificationPresenterImpl.h) Then you could remain the WTF::String variable on line 105 to domainString. Alternatively, you could rename domainStr to domainStdString or domainString and then update the declaration of this function in NotificationPresenterImpl.h. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:123 > + // Because we are using a modal dialog to request permission, it's probably impossible to cancel it. I don't understand how canceling the modal dialog is "probably impossible". It's either possible or not. Nit: I suggest adding a "return" statement here instead of just falling off the end of the function. Even though both calling return here and falling off the end of the function have the same effect, I feel that adding return here makes the body of this function less empty and hence improves the readability of this file as you read through this function. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:130 > + Nit: I suggest removing this empty line since it creates as visual separation that conflicts with the relationship the comment on line 128-129 is trying to build with line 131. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:137 > +// This function will be called by platform side. Nit: This comment doesn't read well. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:138 > +void NotificationPresenterImpl::notificationClicked(const std::string& idStr) Nit: idStr => id (this matches the name of this parameter in the declaration of this function in NotificationPresenterImpl.h) Then you could remain the WTF::String variable on line 141 to idString. Alternatively, you could rename idStr to idStdString or idString and then update the declaration of this function in NotificationPresenterImpl.h. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:147 > + for (; iter != m_notifications.end(); ++iter) > + if (iter->second == id && iter->first->scriptExecutionContext()) { > + iter->first->dispatchEvent(Event::create(eventNames().clickEvent, false, true)); > + m_notifications.remove(iter); > + } This for-loop needs braces since its body is more than a single line. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:24 > +#include "HashMap.h" This should be: #include <wtf/HashMap.h> > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:25 > +#include "HashSet.h" This should be: #include <wtf/HashSet.h> > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:26 > +#include "NotificationAckListener.h" This should be: #include <NotificationAckListener.h> > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:28 > +#include "NotificationPresenterBlackBerry.h" Do we need to include this file here? It should be sufficient to only include this file in NotificationPresenterImpl.cpp (which we already do) and then forward declare BlackBerry::Platform::NotificationPresenterBlackBerry. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:29 > +#include "PlatformString.h" Instead of including this file here, can we include this file in NotificationPresenterImpl.cpp and just forward declare WTF::String in this file? > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:30 > +#include "StringHash.h" Do we need to include this file here? It should be sufficient to only include this file in NotificationPresenterImpl.cpp (which we already do). > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:37 > + Remove this empty line as it doesn't improve the readability of this class declaration. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:58 > + // Cancel all outstanding requests for the ScriptExecutionContext This comment seems inane given the name of this function. I suggest removing it. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:63 > + Remove this empty line. There's already an empty line below and one empty line is sufficient to visually separate these functions. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:65 > + // Interfaces inherits from NotificationAckListener: inherits => inherited : => . > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:75 > + typedef HashMap<RefPtr<WebCore::Notification>, WTF::String> NotificationMap; Do we need the WTF:: prefix? Nit: I suggest adding adding and empty line above this line to increase the readability of the private section of class by creating a visual separation between the OwnPtr and this typedef which is referenced in line 76 (below). > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:77 > + typedef HashMap<RefPtr<WebCore::ScriptExecutionContext>, RefPtr<WebCore::VoidCallback> > PermissionRequestMap; Nit: I suggest adding adding and empty line above this line to increase the readability of the private section of class by creating a visual separation between the m_notifications and this typedef which is referenced in line 78 (below). > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:79 > + HashSet<WTF::String> m_allowedDomains; Do we need the WTF:: prefix?

Comment on attachment 117108 [details] Patch for notification support. V2. View in context: https://bugs.webkit.org/attachment.cgi?id=117108&action=review >> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:72 >> + // FIXME: strip to one line > > Nit: strip => Strip > > Also, comments should be full sentences that end in a period. > > I'm not entirely clear what this comment means. I take it that either notification->contents().title() or notification->contents().body() may contain some kind of line delimiter character(s) (e.g. new line characters) and that we want to remove such characters such that the message is ultimately displayed as one line of text. Yes. That's what I mean. :) >> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:30 >> +#include "StringHash.h" > > Do we need to include this file here? It should be sufficient to only include this file in NotificationPresenterImpl.cpp (which we already do). Yes. StringHash.h is needed in HashSet<WTF::String> instantiation. >> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:58 >> + // Cancel all outstanding requests for the ScriptExecutionContext > > This comment seems inane given the name of this function. I suggest removing it. This comment is copied from its base class, I think I'd better remain it.

Created attachment 117134 [details] Patch for notification support. V3. Modified according to Daniel's comments.

Attachment 117134 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/..." exit_code: 1 Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 117136 [details] Patch for notification support. V4. Sort the headers in check-webkit-style sorting order. (Different from linux sort tool sorting order). linux sort tool: 25 #include <NotificationPresenterBlackBerry.h> 26 #include <NotificationPresenter.h> check-webkit-style: 25 #include <NotificationPresenter.h> 26 #include <NotificationPresenterBlackBerry.h> A bug?

Comment on attachment 117136 [details] Patch for notification support. V4. View in context: https://bugs.webkit.org/attachment.cgi?id=117136&action=review Looks quite close to being finished, some nits, so r- for now :) > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:52 > +bool NotificationPresenterImpl::show(Notification* n) For consistency needs (Notification* notification), see notificationObjectDestroyed. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:77 > +void NotificationPresenterImpl::cancel(Notification* n) Ditto. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:126 > + // FIXME: Should store the permission information into file permenently instead of in m_allowedDomains. Typo -> permanently > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:56 > + // Cancel all outstanding requests for the ScriptExecutionContext misses '.' at the end.

Created attachment 118335 [details] Patch

Hi Dan, (In reply to comment #5) > > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:73 > > + message = notification->contents().title() + ": " + notification->contents().body(); > > We should use makeString() (*) here: > > message = makeString(notification->contents().title(), ": ", notification->contents().body()); > > (*) Defined in JavaScriptCore/wtf/text/StringConcatenate.h: <http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/text/StringConcatenate.h?rev=98624#L621>. Just a side note: We have an optimized operator+ template version, that I worked on some months ago, that delivers the same performance as using makeString. In fact StringAppend<T, StringAppend<T> > operator+(StringAppend&) uses makeString internally. If these are WTF::Strings, using notifcation->contents().title + ": " .. is completely fine, this won't lead to a realloc, instead the final string will be built using makeString().

Comment on attachment 118335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118335&action=review Looks good to me, still some nits that lead to r-. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:34 > +NotificationPresenterImpl* NotificationPresenterImpl::s_instance = 0; No need for this, just move the static s_instance declaration into ::instance() - this way it doesn't need to be present in the header as well. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:36 > +NotificationPresenter* NotificationPresenterImpl::instance() Why is this named *Impl - we generally try to avoid this idiom in WebCore, but as this is WebKit it might be fine. Do you plan to add more *Impl.cpp named files? > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:55 > + if (NotificationPresenter::PermissionAllowed != checkPermission(notification->scriptExecutionContext())) This reads awkward, I'd swap the order here. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:70 > + message = makeString(n->contents().title(), ": ", n->contents().body()); As I mentioned to Dan before, your old variant was fine as well, you could revert to it, this is no gain. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:83 > + if (m_notifications.contains(n)) { > + m_platformPresenter->cancel(std::string(m_notifications.get(n).utf8().data())); > + m_notifications.remove(n); This idiom is slow, don't use contains to check, and then remove to query the hash table again. Instead use m_notifications.find(n) which will return you an iterator, if it's map.end() you can early exit, otherwhise you can cancel the presenter and call remove(it); > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:94 > +{ No ASSERT(context) here? > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:103 > + PermissionRequestMap::iterator iter = m_permissionRequests.begin(); s/iter/it/ - as this is used all over WebCore in this way :-) About the only tolerated abbreviations that we use. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:104 > + for (; iter != m_permissionRequests.end(); ++iter) { If this is hot code, you could cache m_permissionRequests.end(), but I doubt it. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:105 > + if (iter->first->url().host() == domainString) { Early continue; here if it doesn't match. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:121 > + return; Unncessary. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:126 > + // FIXME: Should store the permission information into file permanently instead of in m_allowedDomains. ASSERT(context) ? > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:139 > + NotificationMap::iterator iter = m_notifications.begin(); s/iter/it/

Created attachment 118752 [details] Patch for notification support. V6.

Comment on attachment 118335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118335&action=review >> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:121 >> + return; > > Unncessary. I added this according Daniel's comment. :P

Created attachment 118939 [details] Patch for notification support. V7. Changed according Nikolas' comments.

Comment on attachment 118335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118335&action=review >> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:36 >> +NotificationPresenter* NotificationPresenterImpl::instance() > > Why is this named *Impl - we generally try to avoid this idiom in WebCore, but as this is WebKit it might be fine. Do you plan to add more *Impl.cpp named files? There is already another header file: ./Source/WebCore/notifications/NotificationPresenter.h, If rename NotificationPresenterImpl.h to NotificationPresenter.h, it will conflict with the one in WebCore. So, I keep the original names. >>> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:121 >>> + return; >> >> Unncessary. > > I added this according Daniel's comment. :P I added this according Daniel's comment. :P

Created attachment 118944 [details] Patch for notification support. V8. Contains() -> find()

(In reply to comment #17) > (From update of attachment 118335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118335&action=review > > >> Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:36 > >> +NotificationPresenter* NotificationPresenterImpl::instance() > > > > Why is this named *Impl - we generally try to avoid this idiom in WebCore, but as this is WebKit it might be fine. Do you plan to add more *Impl.cpp named files? > > There is already another header file: ./Source/WebCore/notifications/NotificationPresenter.h, If rename NotificationPresenterImpl.h to NotificationPresenter.h, it will conflict with the one in WebCore. So, I keep the original names. Oh okay, these are the information that are supposed to be in the ChangeLog. Next time someone looks at this, wondering why it was named this way :-)

Comment on attachment 118944 [details] Patch for notification support. V8. View in context: https://bugs.webkit.org/attachment.cgi?id=118944&action=review Almost there, I still have some style nitpicks, and give r- as the ChangeLog doesn't contain any information. At least one line should be added. > Source/WebKit/ChangeLog:7 > + Please add at least one or two lines to the ChangeLog, sth. like "Initial upstreaming, no new tests." > Source/WebKit/ChangeLog:20 > + * blackberry/WebCoreSupport/NotificationPresenterImpl.h: Added. You could explain the file naming here. I still don't see why we don't name it NotificationPresenterBlackBerry.h, or sth. similar. But it's okay for now. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:23 > + One newline too much. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:33 > + One newline too much. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:39 > + s_instance = new NotificationPresenterImpl(); s/()// > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:61 > + if (checkPermission(notification->scriptExecutionContext()) != NotificationPresenter::PermissionAllowed) > + return false; > + > + RefPtr<Notification> n = notification; > + if (m_notifications.contains(n)) > + return false; Which case is simpler to compute? I assume the latter, so we should swap these. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:126 > + return; Even if Dan suggested to add this, I'm suggesting to remove this :-) There is no benefit of adding this, and it's also leading to compiler warnings under certain circumstances IIRC. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:23 > + This newline can be removed. > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.h:84 > + This newline can be removed.

Created attachment 118975 [details] Patch for notification support. V9. Thanks, Nikolas. I updated my patch according to your comments.

Comment on attachment 118975 [details] Patch for notification support. V9. View in context: https://bugs.webkit.org/attachment.cgi?id=118975&action=review Nice job! r=me, with one last comment, just go ahead and land it :-) > Source/WebKit/blackberry/WebCoreSupport/NotificationPresenterImpl.cpp:134 > + One newline too much.

Committed r102754: <http://trac.webkit.org/changeset/102754>

Thanks, Dan, Rob, Niko and Leo.