WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Updated patch based on rob's comment.
(71.02 KB, patch)
2012-08-20 19:55 PDT
,
Lyon Chen
rwlbuis
: review-
Details
Formatted Diff
Diff
Update patch based on rwlbuis' comments.
(70.66 KB, patch)
2012-08-21 10:50 PDT
,
Lyon Chen
no flags
Details
Formatted Diff
Diff
Patch
(3.07 KB, patch)
2012-08-22 08:12 PDT
,
Rob Buis
yong.li.webkit
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 159936
[details]
Patch
Rob Buis
Comment 15
2012-08-22 08:31:29 PDT
Committed
r126303
: <
http://trac.webkit.org/changeset/126303
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug