Righ now the RSS handling in WebKit BlackBerry portal is handled in platform level, but it should be in WebKit layer, so it can use the TextEncoding codes of WebKit, thus avoid inconsistency of text encoding handling.
Created attachment 159465 [details] patch for 93496.
+Yong, Rob, Joe.
Comment on attachment 159465 [details] patch for 93496. View in context: https://bugs.webkit.org/attachment.cgi?id=159465&action=review Some minor issues so far and you need a ChangeLog. > Source/WebCore/platform/network/blackberry/rss/RSSGenerator.h:22 > +#include <wtf/text/WTFString.h> It does not seem like you need this include. > Source/WebCore/platform/network/blackberry/rss/RSSParserBase.cpp:113 > + WTF::String text; You do not need this WTF:: prefix for String.
(In reply to comment #3) > Some minor issues so far and you need a ChangeLog. Will add ChangeLog. > > > Source/WebCore/platform/network/blackberry/rss/RSSParserBase.cpp:113 > > + WTF::String text; > > You do not need this WTF:: prefix for String. Will change accordingly. Thanks!
Created attachment 159596 [details] Updated patch based on rob's comment.
Comment on attachment 159596 [details] Updated patch based on rob's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=159596&action=review Looks good, can do with some more cleanup. > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:37 > +#include "rss/RSSFilterStream.h" I am not sure that you need to do this, the PlatformBlackBerry.cmake change suggests you can leave the rss/ off. > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:62 > +static inline bool isControlCharacter(int data) I wonder if there is a stdlib function for this. > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:67 > +static int isspaceSafe(int ch) What is meant here by "Safe"? > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:117 > + for (size_t start = 0, delimiter = 0;delimiter != std::string::npos; start = delimiter + 1) { Fix spacing please ; delimiter > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:326 > + || (!strcasecmp(language, "en-US"))) No need to wrap this line. > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:667 > + // make sure the headers are never sent twice Please try to make all comments end with a period and start with a capital. Right now it is a bit inconsistent. > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.h:32 > + TYPE_UNKNOWN = 0, The = 0 is not strictly needed.
(In reply to comment #6) > I am not sure that you need to do this, the PlatformBlackBerry.cmake change suggests you can leave the rss/ off. Yes, forgot this. > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:62 > > +static inline bool isControlCharacter(int data) > > I wonder if there is a stdlib function for this. Somehow I forgot to remove it as it is not used now, I use isASCIIPrintable() in where it was used. > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:67 > > +static int isspaceSafe(int ch) > > What is meant here by "Safe"? The "Safe" means just the lowest byte is checked. But I agree it is not very descriptive and I am open to better names. > > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:117 > > + for (size_t start = 0, delimiter = 0;delimiter != std::string::npos; start = delimiter + 1) { > > Fix spacing please ; delimiter Will change. > > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:326 > > + || (!strcasecmp(language, "en-US"))) > > No need to wrap this line. Will change. > Please try to make all comments end with a period and start with a capital. Right now it is a bit inconsistent. Will change. > > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.h:32 > > + TYPE_UNKNOWN = 0, > > The = 0 is not strictly needed. It seems quite some enum in WebKit has the first value = 0 style. And in all my codes I use this style. So I am not sure whether I should change it. But if you feel remove this "= 0," is desired, I can remove it.
> > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.cpp:67 > > > +static int isspaceSafe(int ch) > > > > What is meant here by "Safe"? > > The "Safe" means just the lowest byte is checked. But I agree it is not very descriptive and I am open to better names. Something like isASCIISpaceLowerByte would be better. > > > Source/WebCore/platform/network/blackberry/rss/RSSFilterStream.h:32 > > > + TYPE_UNKNOWN = 0, > > > > The = 0 is not strictly needed. > > It seems quite some enum in WebKit has the first value = 0 style. And in all my codes I use this style. So I am not sure whether I should change it. But if you feel remove this "= 0," is desired, I can remove it. Yes please remove the = 0, also use camelCase for enum values.
Created attachment 159723 [details] Update patch based on rwlbuis' comments.
Comment on attachment 159723 [details] Update patch based on rwlbuis' comments. Looks good.
Comment on attachment 159723 [details] Update patch based on rwlbuis' comments. Clearing flags on attachment: 159723 Committed r126230: <http://trac.webkit.org/changeset/126230>
All reviewed patches have been landed. Closing bug.
Reopening, I found some cleanup possibilities.
Created attachment 159936 [details] Patch
Committed r126303: <http://trac.webkit.org/changeset/126303>