Bug 99248 – [BlackBerry] Adapt to Platform API changes in string handling

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit BlackBerry

[George Staikos]

String handling has changed to use BlackBerry::Platform::String on BlackBerry.  Update WebKit accordingly, removing char* and std::string usage.

[George Staikos]

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.

[WebKit Review Bot]

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.

[George Staikos]

Created attachment 168554 [details]
Large patch to fix all API usage and support the new string class

[Benjamin Poulain]

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()?

[George Staikos]

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

[George Staikos]

Created attachment 168583 [details]
Follow-on patch to change m_mode to AtomicString

[Joe Mason]

Created attachment 168594 [details]
Follow-on patch to fix a function signature that was missed

[WebKit Review Bot]

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>

[WebKit Review Bot]

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>

[George Staikos]

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

[Benjamin Poulain]

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.