RESOLVED FIXED 93496
[BlackBerry] Add RSS content handling support
https://bugs.webkit.org/show_bug.cgi?id=93496
Summary [BlackBerry] Add RSS content handling support
Lyon Chen
Reported 2012-08-08 10:45:26 PDT
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.
Attachments
patch for 93496. (70.54 KB, patch)
2012-08-20 10:13 PDT, Lyon Chen
rwlbuis: review-
Updated patch based on rob's comment. (71.02 KB, patch)
2012-08-20 19:55 PDT, Lyon Chen
rwlbuis: review-
Update patch based on rwlbuis' comments. (70.66 KB, patch)
2012-08-21 10:50 PDT, Lyon Chen
no flags
Patch (3.07 KB, patch)
2012-08-22 08:12 PDT, Rob Buis
yong.li.webkit: review+
Lyon Chen
Comment 1 2012-08-20 10:13:19 PDT
Created attachment 159465 [details] patch for 93496.
Lyon Chen
Comment 2 2012-08-20 10:16:10 PDT
+Yong, Rob, Joe.
Rob Buis
Comment 3 2012-08-20 10:31:44 PDT
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.
Lyon Chen
Comment 4 2012-08-20 11:14:12 PDT
(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!
Lyon Chen
Comment 5 2012-08-20 19:55:11 PDT
Created attachment 159596 [details] Updated patch based on rob's comment.
Rob Buis
Comment 6 2012-08-21 08:21:36 PDT
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.
Lyon Chen
Comment 7 2012-08-21 09:58:16 PDT
(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.
Rob Buis
Comment 8 2012-08-21 10:18:40 PDT
> > > 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.
Lyon Chen
Comment 9 2012-08-21 10:50:23 PDT
Created attachment 159723 [details] Update patch based on rwlbuis' comments.
Rob Buis
Comment 10 2012-08-21 11:11:46 PDT
Comment on attachment 159723 [details] Update patch based on rwlbuis' comments. Looks good.
WebKit Review Bot
Comment 11 2012-08-21 17:14:21 PDT
Comment on attachment 159723 [details] Update patch based on rwlbuis' comments. Clearing flags on attachment: 159723 Committed r126230: <http://trac.webkit.org/changeset/126230>
WebKit Review Bot
Comment 12 2012-08-21 17:14:25 PDT
All reviewed patches have been landed. Closing bug.
Rob Buis
Comment 13 2012-08-22 08:09:59 PDT
Reopening, I found some cleanup possibilities.
Rob Buis
Comment 14 2012-08-22 08:12:30 PDT
Rob Buis
Comment 15 2012-08-22 08:31:29 PDT
Note You need to log in before you can comment on or make changes to this bug.