[webkit-reviews] review denied: [Bug 73194] [BlackBerry] Add notification support for the BlackBerry port : [Attachment 117108] Patch for notification support. V2.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 29 22:12:20 PST 2011
Daniel Bates <dbates at webkit.org> has denied Robin Qiu
<robin.qiu at torchmobile.com.cn>'s request for review:
Bug 73194: [BlackBerry] Add notification support for the BlackBerry port
https://bugs.webkit.org/show_bug.cgi?id=73194
Attachment 117108: Patch for notification support. V2.
https://bugs.webkit.org/attachment.cgi?id=117108&action=review
------- Additional Comments from Daniel Bates <dbates at webkit.org>
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/StringConc
atenate.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?
More information about the webkit-reviews
mailing list