Bug 93496 – [BlackBerry] Add RSS content handling support

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Lyon Chen]

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.

[Lyon Chen]

Created attachment 159465 [details]
patch for 93496.

[Lyon Chen]

+Yong, Rob, Joe.

[Rob Buis]

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]

(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]

Created attachment 159596 [details]
Updated patch based on rob's comment.

[Rob Buis]

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]

(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]

> > > 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]

Created attachment 159723 [details]
Update patch based on rwlbuis' comments.

[Rob Buis]

Comment on attachment 159723 [details]
Update patch based on rwlbuis' comments.

Looks good.

[WebKit Review Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Rob Buis]

Reopening, I found some cleanup possibilities.

[Rob Buis]

Created attachment 159936 [details]
Patch

[Rob Buis]

Committed r126303: <http://trac.webkit.org/changeset/126303>