[Webkit-unassigned] [Bug 73194] [BlackBerry] Add notification support for the BlackBerry port
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 29 22:12:21 PST 2011
https://bugs.webkit.org/show_bug.cgi?id=73194
Daniel Bates <dbates at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #117108|review? |review-
Flag| |
--- Comment #5 from Daniel Bates <dbates at webkit.org> 2011-11-29 22:12:21 PST ---
(From update of attachment 117108)
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?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list