Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1382583002:
Add toStdString methods to CString and WTF::String (Closed)
|
Created:
5 years, 2 months ago by Primiano Tucci (use gerrit) Modified:
4 years, 10 months ago CC:
abarth-chromium, blink-reviews, blink-reviews-wtf_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd toStdString methods to CString and WTF::String
Follow-up CL to:
[blink-dev] How to call Chromium's methods that take std::strings
Adds a toUTF8StdString() method to WTF::String and
a toStdString() method to CString.
BUG=
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add include #
Total comments: 4
Patch Set 3 : Switch to implicit CString -> std::string conversion #Patch Set 4 : Fix tests #Patch Set 5 : Back to String::toUTF8StdString #
Dependent Patchsets: Messages
Total messages: 36 (3 generated)
primiano@chromium.org changed reviewers: + esprehn@chromium.org, haraken@chromium.org
haraken@chromium.org changed reviewers: + tkent@chromium.org
+tkent Looks good to me.
mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com
https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/CString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... 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/w... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8() const; why not toStdString() ?
https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/CString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... 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/w... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... 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/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/CString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/CString.h:69: return std::string(data(), length()); On 2015/09/30 15:08:28, Primiano Tucci wrote: > 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] Right. CStringBuffer's length != its size, sorry for the hassle.. https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... 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/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8() const; On 2015/09/30 15:58:39, Mikhail wrote: > 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? 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() ?
On 2015/09/30 16:03:37, Primiano Tucci wrote: > https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/wtf/text/WTFString.h (right): > > https://codereview.chromium.org/1382583002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/wtf/text/WTFString.h:182: std::string toUTF8() const; > On 2015/09/30 15:58:39, Mikhail wrote: > > 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? > > 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() ? I'm fine with that, should it accept UTF8ConversionMode?
https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/WTFString.cpp (right): https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Sour... 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.
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/Sour... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Sour... 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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/WTFString.h (right): https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Sour... 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.
On 2015/10/01 at 07:34:51, primiano wrote: > https://codereview.chromium.org/1382583002/diff/20001/third_party/WebKit/Sour... > 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.
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/Sour... > > 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 .
>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())
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?
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.
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?
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
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. :)
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.
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.
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.
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?
PING esprehn@, are you okay with the plan tkent proposed in #24?
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?
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?
On 2015/10/05 at 19:21:45, primiano wrote: > 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? Blink should not be using std::string for 99% of cases, this API is not for general usage.
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?
tkent/esprehn can you please take a look now? I followed the suggestion in #24, and switched to use UTF8StringAdaptor. Thanks
looks good to me.
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?
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?
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.
On 2015/10/09 at 09:18:39, primiano wrote: > 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. 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
