Bug 55567 – JavaString API should be implementable by both JSC and V8

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

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.

[Steve Block]

Created attachment 84407 [details]
Patch

[David Levin]

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

[Steve Block]

Created attachment 84436 [details]
Patch

[Jeremy Orlow]

Comment on attachment 84436 [details]
Patch

r=me

[David Levin]

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.

[Steve Block]

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

[Steve Block]

Committed r80156: <http://trac.webkit.org/changeset/80156>

[Steve Block]

Patch broke build and was rolled out - see Bug 55604

[Steve Block]

Created attachment 84465 [details]
Patch

[Steve Block]

Fixed a couple of call sites inside LOG macros that I'd missed in the previous patch.

[WebKit Commit Bot]

Comment on attachment 84465 [details]
Patch

Clearing flags on attachment: 84465

Committed r80222: <http://trac.webkit.org/changeset/80222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.