Bug 55567 - JavaString API should be implementable by both JSC and V8
Summary: JavaString API should be implementable by both JSC and V8
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 55604
Blocks: 55566
  Show dependency treegraph
 
Reported: 2011-03-02 03:17 PST by Steve Block
Modified: 2011-03-03 02:51 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.04 KB, patch)
2011-03-02 05:17 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2011-03-02 10:22 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (9.45 KB, patch)
2011-03-02 14:15 PST, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2011-03-02 03:17:25 PST
Currently, the JavaString API includes 'operator UString() const' as a means of accessing the UTF-16 encoded version of the string. However, UString is a JSC-specific type, so this can only be implemented with JSC.

We should replace this operator with a method that is independent of the script engine.
Comment 1 Steve Block 2011-03-02 05:17:40 PST
Created attachment 84407 [details]
Patch
Comment 2 David Levin 2011-03-02 10:04:39 PST
Comment on attachment 84407 [details]
Patch

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

> Source/WebCore/bridge/jni/jsc/JavaStringJSC.h:72
> +    PassRefPtr<StringImpl> impl() const { return m_impl; }

"If a function’s result is an object, but ownership is not being transferred, the result should be a raw pointer. This includes most getter functions." -- http://www.webkit.org/coding/RefPtr.html

In other words, this should return StringImpl*.
Comment 3 Steve Block 2011-03-02 10:22:03 PST
Created attachment 84436 [details]
Patch
Comment 4 Jeremy Orlow 2011-03-02 11:23:32 PST
Comment on attachment 84436 [details]
Patch

r=me
Comment 5 David Levin 2011-03-02 11:27:23 PST
I reviewed at the same time as jorlow. fwiw, here was my comment:

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

On a syntax/style level the patch looks fine, but I don't understand what this patch accomplishes.

The stated intent is for something which both v8 and jsc can implement but the v8 implementation is non-existant: "StringImpl* impl() const { ASSERT_NOT_REACHED(); return 0; }"

> Source/WebCore/bridge/jni/v8/JavaStringV8.h:33
> +#include <wtf/text/StringImpl.h>

I believe a fwd declaration with suffice instead of including the header.
Comment 6 Steve Block 2011-03-02 12:07:27 PST
> The stated intent is for something which both v8 and jsc can implement but
> the v8 implementation is non-existant:
The primary intention is to avoid needless ifdefs in the JavaString API. This ifdef should have been avoided when the V8 implementation was first upstreamed.

A real implementation of the method will be provided as part of Bug 55566, for which this change is a requirement. I also have other refactorings coming up that will require JavaString to have a UTF-16 getter.

> I believe a fwd declaration with suffice instead of including the header.
Will do.
Comment 7 Steve Block 2011-03-02 12:23:22 PST
Committed r80156: <http://trac.webkit.org/changeset/80156>
Comment 8 Steve Block 2011-03-02 13:04:17 PST
Patch broke build and was rolled out - see Bug 55604
Comment 9 Steve Block 2011-03-02 14:15:25 PST
Created attachment 84465 [details]
Patch
Comment 10 Steve Block 2011-03-02 14:16:31 PST
Fixed a couple of call sites inside LOG macros that I'd missed in the previous patch.
Comment 11 WebKit Commit Bot 2011-03-03 02:51:21 PST
Comment on attachment 84465 [details]
Patch

Clearing flags on attachment: 84465

Committed r80222: <http://trac.webkit.org/changeset/80222>
Comment 12 WebKit Commit Bot 2011-03-03 02:51:27 PST
All reviewed patches have been landed.  Closing bug.