Add WTFString::toUTF8() method (issue 1382583002 by primiano@chromium.org)

[prim...@chromium.org]

Reviewers: haraken, esprehn,

Description:
Add WTFString::toUTF8() method

Follow-up CL to:
[blink-dev] How to call Chromium's methods that take std::strings

Adds a toUTF8() method to WTFString and recursively a toStdString()
method to CString.
This is effectively a backport from github.com/flutter + tests.

BUG=

Please review this at https://codereview.chromium.org/1382583002/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+24, -0 lines):
M third_party/WebKit/Source/wtf/text/CString.h
M third_party/WebKit/Source/wtf/text/CStringTest.cpp
M third_party/WebKit/Source/wtf/text/WTFString.h
M third_party/WebKit/Source/wtf/text/WTFString.cpp
M third_party/WebKit/Source/wtf/text/WTFStringTest.cpp


Index: third_party/WebKit/Source/wtf/text/CString.h
diff --git a/third_party/WebKit/Source/wtf/text/CString.h
b/third_party/WebKit/Source/wtf/text/CString.h
index
08e51027e774748d32f6336d76d19f32af7a41e7..cec3fce820e75905da27600e8abedebf27d0e4b1
100644
--- a/third_party/WebKit/Source/wtf/text/CString.h
+++ b/third_party/WebKit/Source/wtf/text/CString.h
@@ -64,6 +64,11 @@ public:
CString(CStringBuffer* buffer) : m_buffer(buffer) { }
static CString newUninitialized(size_t length, char*& characterBuffer);

+ std::string toStdString() const
+ {
+ return std::string(data(), length());
+ }
+
const char* data() const
{
return m_buffer ? m_buffer->data() : 0;
Index: third_party/WebKit/Source/wtf/text/CStringTest.cpp
diff --git a/third_party/WebKit/Source/wtf/text/CStringTest.cpp
b/third_party/WebKit/Source/wtf/text/CStringTest.cpp
index
087cbb63634c008046a5437ad0d11cae33e1e7a8..a428bdb0707cdb87f60bdc52c5ed963f7e5e4c48
100644
--- a/third_party/WebKit/Source/wtf/text/CStringTest.cpp
+++ b/third_party/WebKit/Source/wtf/text/CStringTest.cpp
@@ -108,7 +108,9 @@ TEST(CStringTest, CopyOnWrite)
string.mutableData()[3] = 'K';
EXPECT_TRUE(string != copy);
EXPECT_STREQ("WebKit", string.data());
+ EXPECT_EQ("WebKit", string.toStdString());
EXPECT_STREQ(initialString, copy.data());
+ EXPECT_EQ(initialString, copy.toStdString());
}

TEST(CStringTest, Comparison)
Index: third_party/WebKit/Source/wtf/text/WTFString.cpp
diff --git a/third_party/WebKit/Source/wtf/text/WTFString.cpp
b/third_party/WebKit/Source/wtf/text/WTFString.cpp
index
6fee9d06de8ce8269a1a0d9ad21a820acf6fc3d9..7433d3f5497624fdd140dc7bc202706c144d7b42
100644
--- a/third_party/WebKit/Source/wtf/text/WTFString.cpp
+++ b/third_party/WebKit/Source/wtf/text/WTFString.cpp
@@ -836,6 +836,11 @@ CString String::utf8(UTF8ConversionMode mode) const
return CString(bufferVector.data(), buffer - bufferVector.data());
}

+std::string String::toUTF8() const
+{
+ return utf8().toStdString();
+}
+
String String::make8BitFrom16BitSource(const UChar* source, size_t length)
{
if (!length)
Index: third_party/WebKit/Source/wtf/text/WTFString.h
diff --git a/third_party/WebKit/Source/wtf/text/WTFString.h
b/third_party/WebKit/Source/wtf/text/WTFString.h
index
256abc6a131a9904461570997cf74da03650d099..b2ee6852357237905b3e7909135cd467b2389b43
100644
--- a/third_party/WebKit/Source/wtf/text/WTFString.h
+++ b/third_party/WebKit/Source/wtf/text/WTFString.h
@@ -179,6 +179,8 @@ public:
CString latin1() const;
CString utf8(UTF8ConversionMode = LenientUTF8Conversion) const;

+ std::string toUTF8() const;
+
UChar operator[](unsigned index) const
{
if (!m_impl || index >= m_impl->length())
Index: third_party/WebKit/Source/wtf/text/WTFStringTest.cpp
diff --git a/third_party/WebKit/Source/wtf/text/WTFStringTest.cpp
b/third_party/WebKit/Source/wtf/text/WTFStringTest.cpp
index
66cf6370575e91a79756585de2b5ad831776f677..0f142fcd6b955f65eb1a0cbeee107dfa457ec02a
100644
--- a/third_party/WebKit/Source/wtf/text/WTFStringTest.cpp
+++ b/third_party/WebKit/Source/wtf/text/WTFStringTest.cpp
@@ -347,4 +347,14 @@ TEST(StringTest, Lower)
EXPECT_STREQ("link",
String::fromUTF8("LIN\xE2\x84\xAA").lower().utf8().data());
}

+TEST(StringTest, ToStdString)
+{
+ char kAsciiChars[] = "1234";
+ EXPECT_EQ(std::string(kAsciiChars), String(kAsciiChars).toUTF8());
+
+ UChar kUTF16Chars[] = {0x20AC, 0xD801, 0};
+ EXPECT_EQ(std::string("\xE2\x82\xAC\xED\xA0\x81"),
String(kUTF16Chars).toUTF8());
+
+}
+
} // namespace WTF

[har...@chromium.org]

+tkent

Looks good to me.


https://codereview.chromium.org/1382583002/

[mikhail.p...@intel.com]

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/CString.h
File third_party/WebKit/Source/wtf/text/CString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/CString.h#newcode69
third_party/WebKit/Source/wtf/text/CString.h:69: return
std::string(data(), length());
length() includes NULL terminator, so should be 'length() - 1'

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h#newcode182
third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8()
const;
why not toStdString() ?

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/CString.h
File third_party/WebKit/Source/wtf/text/CString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/CString.h#newcode69
third_party/WebKit/Source/wtf/text/CString.h:69: return
std::string(data(), length());

On 2015/09/30 14:02:01, Mikhail wrote:
> length() includes NULL terminator, so should be 'length() - 1'

Just tried: String("").length() returns 0 not 1.

If I do that (-1) the unittest (see the new empty one) crashes on an
empty string, with this:

[ RUN ] StringTest.ToStdString
terminate called after throwing an instance of 'std::length_error'
what(): basic_string::_S_create
Received signal 6
#0 0x7f5ae2d16e7b base::debug::(anonymous
namespace)::StackDumpSignalHandler()
#1 0x7f5ae2246340 <unknown>
#2 0x7f5ae1ea7cc9 gsignal
#3 0x7f5ae1eab0d8 abort
#4 0x7f5ae27ba535 __gnu_cxx::__verbose_terminate_handler()
#5 0x7f5ae27b86d6 <unknown>
#6 0x7f5ae27b8703 std::terminate()
#7 0x7f5ae27b8922 __cxa_throw
#8 0x7f5ae280a387 std::__throw_length_error()
#9 0x7f5ae2814222 std::string::_Rep::_S_create()
#10 0x7f5ae2815931 std::string::_S_construct<>()
#11 0x7f5ae28159ed std::string::string()
#12 0x7f5ae2de6ca2 WTF::String::toUTF8()
#13 0x000000606554 WTF::StringTest_ToStdString_Test::TestBody()
#14 0x000000642c77 testing::Test::Run()
#15 0x00000064376a testing::TestInfo::Run()
#16 0x000000643c03 testing::TestCase::Run()
#17 0x00000064ad29 testing::internal::UnitTestImpl::RunAllTests()
#18 0x00000064a9ce testing::UnitTest::Run()
#19 0x00000060abad base::TestSuite::Run()
#20 0x000000617ad3 base::(anonymous
namespace)::LaunchUnitTestsInternal()
#21 0x0000006179aa base::LaunchUnitTests()
#22 0x00000060aaa4 base::RunUnitTestsUsingBaseTestSuite()
#23 0x7f5ae1e92ec5 __libc_start_main
#24 0x00000042b9d0 <unknown>
r8: 000000000000000a r9: 00007f5ae2c1a780 r10: 0000000000000008 r11:
0000000000000206
r12: 000026a8e78613e0 r13: 000026a8e785de00 r14: 00007ffca880eb78 r15:
000026a8e785afc0
di: 00000000000079b1 si: 00000000000079b1 bp: 000026a8e78aa498 bx:
00007f5ae2230868
dx: 0000000000000006 ax: 0000000000000000 cx: ffffffffffffffff sp:
00007ffca880e828
ip: 00007f5ae1ea7cc9 efl: 0000000000000206 cgf: 0000000000000033 erf:
0000000000000000
trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h#newcode182
third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8()
const;

On 2015/09/30 14:02:01, Mikhail wrote:
> why not toStdString() ?

AFAIK technically speaking std::string does not mandate a specific
encoding [1], so toStdString wouldn't explain what encoding the caller
should expect.
More specifically, if you construct a WTF::String out of a UTF-16 string
[0x20AC, 0xD801], I could expect a toStdString() to just return a string
containing 4 bytes (0x20, 0xAC, 0xD8, 0x01), which is NOT the case here.
What you get is the equivalent UTF8 encoding of the same string (See the
unittest)

[1] cplusplus.com says "If used to handle sequences of multi-byte or
variable-length characters",
http://www.cplusplus.com/reference/string/string/

https://codereview.chromium.org/1382583002/

[mikhail.p...@intel.com]

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/CString.h
File third_party/WebKit/Source/wtf/text/CString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/CString.h#newcode69
third_party/WebKit/Source/wtf/text/CString.h:69: return
std::string(data(), length());

Right. CStringBuffer's length != its size, sorry for the hassle..

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h#newcode182
third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8()
const;

On 2015/09/30 15:08:28, Primiano Tucci wrote:
> On 2015/09/30 14:02:01, Mikhail wrote:
> > why not toStdString() ?

> AFAIK technically speaking std::string does not mandate a specific
encoding [1],
> so toStdString wouldn't explain what encoding the caller should
expect.
> More specifically, if you construct a WTF::String out of a UTF-16
string
> [0x20AC, 0xD801], I could expect a toStdString() to just return a
string
> containing 4 bytes (0x20, 0xAC, 0xD8, 0x01), which is NOT the case
here. What
> you get is the equivalent UTF8 encoding of the same string (See the
unittest)

> [1] http://cplusplus.com says "If used to handle sequences of

multi-byte or
> variable-length characters",
http://www.cplusplus.com/reference/string/string/

Agree, however having both 'CString utf8()' and 'std::string toUTF8()'
at the same time looks a bit awkward. Also 'to' prefix usually implies
casting to a type and here we just create a new 'std::string' instance.
Maybe 'utf8StdString()' or smth. like that?

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/wtf/text/WTFString.h#newcode182
third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8()
const;

Hmm ok for the StdString part, but I'd keep the to.
In my mind "to" implies a transformation, without to I expect it to be a
trivial operation / casting.
How about toUTF8StdString() ?

https://codereview.chromium.org/1382583002/

[mikhail.p...@intel.com]

I'm fine with that, should it accept UTF8ConversionMode?

https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.cpp
File third_party/WebKit/Source/wtf/text/WTFString.cpp (right):

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.cpp#newcode841
third_party/WebKit/Source/wtf/text/WTFString.cpp:841: return
utf8().toStdString();
callers should just do .utf8().toStdString()

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h#newcode183
third_party/WebKit/Source/wtf/text/WTFString.h:183: std::string toUTF8()
const;
This is confusing with the .utf8() method. I'm also hesitant to do this
yet, we shouldn't be using std::string in almost all blink code.

https://codereview.chromium.org/1382583002/

[tk...@chromium.org]

If we add CString::toStdString(), I think adding String::toUTF8StdString()
is
also reasonable. str.utf8().toStdString() would be verbose a little.

Another idea is to allow implicit conversion from CString to std::string,
and
use |str.utf8()| for std::string arguments.

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h#newcode183
third_party/WebKit/Source/wtf/text/WTFString.h:183: std::string toUTF8()
const;

On 2015/09/30 at 18:15:39, esprehn wrote:
> This is confusing with the .utf8() method. I'm also hesitant to do
this yet, we shouldn't be using std::string in almost all blink code.

At least, the function should have a document that we should use it only
to pass stings to Chromium.

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h
File third_party/WebKit/Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h#newcode183
third_party/WebKit/Source/wtf/text/WTFString.h:183: std::string toUTF8()
const;

On 2015/10/01 02:09:06, tkent wrote:
> On 2015/09/30 at 18:15:39, esprehn wrote:
> > This is confusing with the .utf8() method. I'm also hesitant to do
this yet,
> we shouldn't be using std::string in almost all blink code.

> At least, the function should have a document that we should use it
only to pass
> stings to Chromium.

So, what is the final take? keep it and document? Or rely on
utf8().toStdString() ?
I think we need to come up with a decision here.
Is there the possibility that in future WTFString.toStdString() could do
something smarter (read: more efficient) than utf8().toStdString() ?
If so I think we should go for keeping here and documenting as tkent
suggested.

https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

On 2015/10/01 at 07:34:51, primiano wrote:

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h
> File third_party/WebKit/Source/wtf/text/WTFString.h (right):

> ...

> So, what is the final take? keep it and document? Or rely on
utf8().toStdString() ?
> I think we need to come up with a decision here.
> Is there the possibility that in future WTFString.toStdString() could do
something smarter (read: more efficient) than utf8().toStdString() ?
> If so I think we should go for keeping here and documenting as tkent
suggested.

I want it long and ugly, almost no one should be using it. :)

I almost wonder if we should just add an operator to CString that's only
implemented in a header in platform/ and where it should actually be used.

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/01 08:28:47, esprehn wrote:
> On 2015/10/01 at 07:34:51, primiano wrote:
> >

https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Source/wtf/text/WTFString.h
> > File third_party/WebKit/Source/wtf/text/WTFString.h (right):
> >
> > ...
> >
> > So, what is the final take? keep it and document? Or rely on
> utf8().toStdString() ?
> > I think we need to come up with a decision here.
> > Is there the possibility that in future WTFString.toStdString() could do
> something smarter (read: more efficient) than utf8().toStdString() ?
> > If so I think we should go for keeping here and documenting as tkent
> suggested.

> I want it long and ugly, almost no one should be using it. :)

asStdStringForChromium()? passToEmbedderAsStdString()?

> I almost wonder if we should just add an operator to CString that's only
> implemented in a header in platform/ and where it should actually be used.

Hmm, the problem is not if you use it in platform/ or somewhere else. The
problem is that you don't want the std::string to be used anywhere in blink
but
only passed to chromium .

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

> Another idea is to allow implicit conversion from CString to std::string,
> and
> use |str.utf8()| for std::string arguments.

Ok I went down this road. Seems the cleanest and relies on the existing
CString
conversion (utf8())


https://codereview.chromium.org/1382583002/

[tk...@chromium.org]

I think Patch 3 is ok if compilation errors are fixed. We can avoid
unnecessary
std::string usage by code reviews.
But probably esprehn doesn't like it?


https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

On 2015/10/02 at 07:52:26, tkent wrote:
> I think Patch 3 is ok if compilation errors are fixed. We can avoid
unnecessary std::string usage by code reviews.
> But probably esprehn doesn't like it?

Yeah code review is not a good way to enforce rules, there's hundreds of
people
contributing code to blink. Indeed std::string and std::vector have already
crept in a couple apis from new developers.

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/02 07:52:26, tkent wrote:
> I think Patch 3 is ok if compilation errors are fixed. We can avoid
unnecessary
> std::string usage by code reviews.
> But probably esprehn doesn't like it?

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/02 at 07:52:26, tkent wrote:
> I think Patch 3 is ok if compilation errors are fixed.

Yeah tests fixed

> Yeah code review is not a good way to enforce rules

I am confused. Are you saying you are against P.S. #3 approach?
If that is the case, would you guys mind agreeing and providing some
direction?

This is blocking other work.
Thanks


https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

On 2015/10/02 at 08:18:52, primiano wrote:
> On 2015/10/02 at 07:52:26, tkent wrote:
> > I think Patch 3 is ok if compilation errors are fixed.
> Yeah tests fixed

> > Yeah code review is not a good way to enforce rules
> I am confused. Are you saying you are against P.S. #3 approach?
> If that is the case, would you guys mind agreeing and providing some
direction?

> This is blocking other work.
> Thanks

What is this blocking? There's no rush to start using base directly in
Blink. :)

https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

Hmm, also none of these patches are using the StringUTF8Adaptor to avoid
mallocing a separate buffer like abarth@ suggested.

I guess that means we'd have to do toUtf8StdString() on WTFString and move
the
code from WebString::utf8() into there, then make WebString::utf8() call
that.

That seems fine to land with a comment saying not to use this outside
platform/,
I don't think we should be calling chromium methods outside platform/ that
take
a std::string yet.

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/02 08:34:21, esprehn wrote:

> What is this blocking? There's no rush to start using base directly in
> Blink.
:)

I'ts not a matter of rush, it's a matter of planning. For my specific case,
there are people requiring new features for tracing (mostly making the
existing
chromium one accessible).
If this (strings) requires more thinking, it's fine, I'll ask them to do the
usual plumbing as we always did. But if there is a possibility that this
goes
through, I'd more likely stall them for a couple of days rather than having
them
landing plumbing code which I'm going to wipe away the day after (That's not
really pleasant, nor a good use of engineering time).
On the other side, I cannot stall their work for a week.

> Hmm, also none of these patches are using the StringUTF8Adaptor to avoid
> mallocing a separate buffer like abarth@ suggested.

Correct me if I am wrong, but it looks like if we want to use
StringUTF8Adaptor
we have to add a new method to WTFString and cannot pass by .utf8() as was
suggested above.
I'm fine with this as long as there is agreement.


https://codereview.chromium.org/1382583002/

[tk...@chromium.org]

I think we can agree with the following approach:
- Based on Patch Set 2
- Rename String::toUTF8() to String::toUTF8StdString()
- It should use StringUTF8Adaptor
- Add comments to toStdString() and toUTF8StdString().

If we'd like to limit the usage of CString::toStdString() and
String::toUTF8StdString() only in Source/platform (this needs a discussion
on
blink-dev), we should add a presubmit check for it later.


https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/02 09:03:34, tkent wrote:
> I think we can agree with the following approach:
> - Based on Patch Set 2
> - Rename String::toUTF8() to String::toUTF8StdString()
> - It should use StringUTF8Adaptor
> - Add comments to toStdString() and toUTF8StdString().

+esprehn, do we agree with this plan?

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

PING esprehn@, are you okay with the plan tkent proposed in #24?



https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

On 2015/10/05 at 15:30:48, primiano wrote:
> PING esprehn@, are you okay with the plan tkent proposed in #24?

Where do you plan to use this? Again, I don't want toUTF8StdString() calls
over
all over, where does this get used, can you share some examples of what code
will use this?

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/05 19:17:17, esprehn wrote:
> On 2015/10/05 at 15:30:48, primiano wrote:
> > PING esprehn@, are you okay with the plan tkent proposed in #24?

> Where do you plan to use this? Again, I don't want toUTF8StdString() calls
over
> all over, where does this get used, can you share some examples of what
> code
> will use this?

Here: crrev.com/1375643002
Even if, I'm not sure I understand: from a process viewpoint, once the
chrome
headers that are usable are be whitelisted in DEPS on a one-by-one basis
(see
haraken doc on blink-dev), why is the specific use case relevant?


https://codereview.chromium.org/1382583002/

[esp...@chromium.org]

Blink should not be using std::string for 99% of cases, this API is not for
general usage.

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/05 19:23:40, esprehn wrote:
> Blink should not be using std::string for 99% of cases, this API is not
> for
> general usage.

I thought what we were discussing here was how to pass strings to chromium
methods that take std::strings, not using std::strings in blink.
Having established that, what is the best way forward?


https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

tkent/esprehn can you please take a look now?
I followed the suggestion in #24, and switched to use UTF8StringAdaptor.

Thanks

https://codereview.chromium.org/1382583002/

[tk...@chromium.org]

looks good to me.


https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

Ok, at this point this is a precondition to lot of cleanup. I can get rid of
~700 lines with this (see crrev.com/1375643002 and crrev.com/1394183003)
Plus I can unblock people doing other memory-infra work in blink (for
instance
crrev.com/1372573003 and crbug.com/524631)
Is there any concern left?
If not can somebody give the green lights to this?

https://codereview.chromium.org/1382583002/

[oj...@chromium.org]

On 2015/10/08 at 18:18:40, primiano wrote:
> Ok, at this point this is a precondition to lot of cleanup. I can get rid
> of
~700 lines with this (see crrev.com/1375643002 and crrev.com/1394183003)
> Plus I can unblock people doing other memory-infra work in blink (for
> instance
crrev.com/1372573003 and crbug.com/524631)
> Is there any concern left?
> If not can somebody give the green lights to this?

Please wait a couple weeks while we figure out the right path forward. I
understand you don't want to block unnecessarily, but it's important that
we do
blink/chromium integration in a way that makes for a good, holistic end
result.
Elliott is on vacation right now and only just became TL for this work a
week
before his vacation. He'll make an initial proposal next week and we can see
what things look like based off the response to that proposal. That OK?

https://codereview.chromium.org/1382583002/

[prim...@chromium.org]

On 2015/10/08 18:53:20, ojan wrote:
> Please wait a couple weeks while we figure out the right path forward. I
> understand you don't want to block unnecessarily, but it's important that
> we
do
> blink/chromium integration in a way that makes for a good, holistic end
result.
> Elliott is on vacation right now and only just became TL for this work a
> week
> before his vacation. He'll make an initial proposal next week and we can
> see
> what things look like based off the response to that proposal. That OK?

It is perfectly OK to say: "this cannot happen now, we need X weeks to
figure
out how to do things properly, please adjust your plans accordingly".
Would have been nice to get this 33 replies ago: it would have avoided me to
write refactoring/cleanup CLs that will go bitrotten and would have avoided
me
to put other people on hold and just tell them "keep plumbing via the glue
layer
as usual".

Super happy to know that you guys are planning a proper way forward.
Just, maybe next time give the NACK message straight away, it will be a
better
use of everybody's time.

Thanks for looking into this.

https://codereview.chromium.org/1382583002/

[oj...@chromium.org]

I understand the frustration. FWIW, organization around this stuff was a bit
messy for a couple weeks and only just landed on Elliott figuring it out.

https://codereview.chromium.org/1382583002/