Bug 99248 - [BlackBerry] Adapt to Platform API changes in string handling
Summary: [BlackBerry] Adapt to Platform API changes in string handling
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: George Staikos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-13 08:05 PDT by George Staikos
Modified: 2014-01-13 21:43 PST (History)
12 users (show)

See Also:


Attachments
Large patch to fix all API usage and support the new string class (187.64 KB, patch)
2012-10-13 08:11 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Large patch to fix all API usage and support the new string class (187.78 KB, patch)
2012-10-13 08:27 PDT, George Staikos
no flags Details | Formatted Diff | Diff
Follow-on patch to change m_mode to AtomicString (1.81 KB, patch)
2012-10-14 07:20 PDT, George Staikos
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
Follow-on patch to fix a function signature that was missed (3.59 KB, patch)
2012-10-14 14:53 PDT, Joe Mason
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description George Staikos 2012-10-13 08:05:53 PDT
String handling has changed to use BlackBerry::Platform::String on BlackBerry.  Update WebKit accordingly, removing char* and std::string usage.
Comment 1 George Staikos 2012-10-13 08:11:10 PDT
Created attachment 168553 [details]
Large patch to fix all API usage and support the new string class

This needs to be done in one shot really, since it's a complete API change.  Basically makes it compile and function.
Comment 2 WebKit Review Bot 2012-10-13 08:14:15 PDT
Attachment 168553 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/As..." exit_code: 1
Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:200:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/blackberry/Api/WebPage_p.h:114:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/blackberry/Api/WebPage.cpp:756:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/blackberry/ResourceRequestBlackBerry.cpp:151:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/blackberry/Api/WebPage.h:100:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/blackberry/Api/WebPageClient.h:202:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit/blackberry/Api/WebPageClient.h:213:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 7 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 George Staikos 2012-10-13 08:27:02 PDT
Created attachment 168554 [details]
Large patch to fix all API usage and support the new string class
Comment 4 Benjamin Poulain 2012-10-13 15:21:05 PDT
Comment on attachment 168554 [details]
Large patch to fix all API usage and support the new string class

View in context: https://bugs.webkit.org/attachment.cgi?id=168554&action=review

> Source/WTF/wtf/text/StringImpl.h:49
> +#if PLATFORM(BLACKBERRY)
> +#include <BlackBerryPlatformString.h>
> +#endif
> +

Why? You do not make any related change it that header.

> Source/WebCore/html/track/TextTrack.cpp:103
> -    , m_mode(disabledKeyword())
> +    , m_mode(disabledKeyword().string())

This is unrelated to the change.

It looks like the proper fix is to make m_mode an AtomicString, you are working against that.

Implicit conversion is a perfectly valid pattern in that case, why do you switch to the method string()?
Comment 5 George Staikos 2012-10-14 06:53:32 PDT
(In reply to comment #4)
> (From update of attachment 168554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168554&action=review
> 
> > Source/WTF/wtf/text/StringImpl.h:49
> > +#if PLATFORM(BLACKBERRY)
> > +#include <BlackBerryPlatformString.h>
> > +#endif
> > +
> 
> Why? You do not make any related change it that header.

  The conversion operator requires it.  Won't compile otherwise.

> > Source/WebCore/html/track/TextTrack.cpp:103
> > -    , m_mode(disabledKeyword())
> > +    , m_mode(disabledKeyword().string())
> 
> This is unrelated to the change.
> 
> It looks like the proper fix is to make m_mode an AtomicString, you are working against that.
> 
> Implicit conversion is a perfectly valid pattern in that case, why do you switch to the method string()?

   Yes, sort of unrelated, but fact is we can't compile with it once we make the change, so sort of related too.  AtomicString sounds like a good choice.
Comment 6 George Staikos 2012-10-14 07:20:30 PDT
Created attachment 168583 [details]
Follow-on patch to change m_mode to AtomicString
Comment 7 Joe Mason 2012-10-14 14:53:25 PDT
Created attachment 168594 [details]
Follow-on patch to fix a function signature that was missed
Comment 8 WebKit Review Bot 2012-10-15 10:31:42 PDT
Comment on attachment 168554 [details]
Large patch to fix all API usage and support the new string class

Clearing flags on attachment: 168554

Committed r131316: <http://trac.webkit.org/changeset/131316>
Comment 9 WebKit Review Bot 2012-10-15 11:14:28 PDT
Comment on attachment 168594 [details]
Follow-on patch to fix a function signature that was missed

Clearing flags on attachment: 168594

Committed r131324: <http://trac.webkit.org/changeset/131324>
Comment 10 George Staikos 2012-10-17 14:19:33 PDT
(In reply to comment #4)
> (From update of attachment 168554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168554&action=review
> 
> > Source/WTF/wtf/text/StringImpl.h:49
> > +#if PLATFORM(BLACKBERRY)
> > +#include <BlackBerryPlatformString.h>
> > +#endif
> > +
> 
> Why? You do not make any related change it that header.
> 
> > Source/WebCore/html/track/TextTrack.cpp:103
> > -    , m_mode(disabledKeyword())
> > +    , m_mode(disabledKeyword().string())
> 
> This is unrelated to the change.
> 
> It looks like the proper fix is to make m_mode an AtomicString, you are working against that.
> 
> Implicit conversion is a perfectly valid pattern in that case, why do you switch to the method string()?

The patch is pending your review.....
Comment 11 Benjamin Poulain 2012-10-17 14:47:14 PDT
Comment on attachment 168583 [details]
Follow-on patch to change m_mode to AtomicString

View in context: https://bugs.webkit.org/attachment.cgi?id=168583&action=review

You should change setMode() and mode() accordingly.

> Source/WebCore/ChangeLog:4
> +        Conert m_mode to be an AtomicString.
> +        https://bugs.webkit.org/show_bug.cgi?id=99248

Conert -> Convert.
This should have its own bug in bugzilla.