Bug 115781 - [BlackBerry] Fix usage of BlackBerry::Platform::String
Summary: [BlackBerry] Fix usage of BlackBerry::Platform::String
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 111729
  Show dependency tree / graph
 
Reported: 2013-05-08 01:51 PDT by Alberto Garcia
Modified: 2013-05-08 15:04 PDT (History)
2 users (show)

See Also:

Attachments
Patch (41.25 KB, patch)
2013-05-08 01:56 PDT, Alberto Garcia 		benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
Patch (47.51 KB, patch)
2013-05-08 04:42 PDT, Alberto Garcia 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 2013-05-08 01:51:43 PDT 
We're creating BlackBerry::Platform::String passing UTF-8 data to its char* constructor, which is interpreted as Latin1.
Comment 1 Alberto Garcia 2013-05-08 01:56:49 PDT 
Created attachment 201044 [details]
Patch
Comment 2 Benjamin Poulain 2013-05-08 02:08:45 PDT 
Comment on attachment 201044 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebSettings.cpp:209
> +    DEFINE_STATIC_LOCAL(AtomicString, WebKitMonospace, ("-webkit-monospace"));
> +    DEFINE_STATIC_LOCAL(AtomicString, WebKitSansSerif, ("-webkit-sans-serif"));
> +    DEFINE_STATIC_LOCAL(AtomicString, WebKitSerif, ("-webkit-serif"));
> +    DEFINE_STATIC_LOCAL(AtomicString, WebKitStandard, ("-webkit-standard"));

Given that "settings" is static local, you will never hit this path again. Why would you define those static?

It also looks like your "WebSettings" class takes a String, not an AtomicString.
Comment 3 Arvid Nilsson 2013-05-08 02:38:47 PDT 
(In reply to comment #2)
> (From update of attachment 201044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201044&action=review
> 
> > Source/WebKit/blackberry/Api/WebSettings.cpp:209
> > +    DEFINE_STATIC_LOCAL(AtomicString, WebKitMonospace, ("-webkit-monospace"));
> > +    DEFINE_STATIC_LOCAL(AtomicString, WebKitSansSerif, ("-webkit-sans-serif"));
> > +    DEFINE_STATIC_LOCAL(AtomicString, WebKitSerif, ("-webkit-serif"));
> > +    DEFINE_STATIC_LOCAL(AtomicString, WebKitStandard, ("-webkit-standard"));
> 
> Given that "settings" is static local, you will never hit this path again. Why would you define those static?
> 
> It also looks like your "WebSettings" class takes a String, not an AtomicString.

One possible reasoning behind this could be:
"-webkit-monospace" will be added into the AtomicString table sooner or later anyway, or could already be in the table at this point.
That depends on if/how -webkit-monospace is used down in WebCore.
Comment 4 Alberto Garcia 2013-05-08 02:55:36 PDT 
Anyway I think Benjamin is right in that it's not necessary to define
these static. I'll update the patch to fix that.

I'll also add a few more changes from a different internal commit that
can also go here.
Comment 5 Alberto Garcia 2013-05-08 04:42:32 PDT 
Created attachment 201057 [details]
Patch

Second try.
Comment 6 Rob Buis 2013-05-08 14:37:33 PDT 
Comment on attachment 201057 [details]
Patch

LGTM.
Comment 7 WebKit Commit Bot 2013-05-08 15:04:44 PDT 
Comment on attachment 201057 [details]
Patch

Clearing flags on attachment: 201057

Committed r149769: <http://trac.webkit.org/changeset/149769>
Comment 8 WebKit Commit Bot 2013-05-08 15:04:47 PDT 
All reviewed patches have been landed.  Closing bug.