Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1513783005:
[Blob] Fix for resetting reader. (Closed)
Description[Blob] Fix for resetting reader.
R=michaeln
BUG=567692
Committed-First-Time: https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2
Cr-Commit-Position-First-Time: refs/heads/master@{#365177}
Committed: https://crrev.com/f01c1228c9cca9b4078443438dfa868f7b972ac2
Cr-Commit-Position: refs/heads/master@{#365316}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Comments #Patch Set 3 : comments #Patch Set 4 : comments #
Total comments: 2
Patch Set 5 : comments #
Total comments: 1
Patch Set 6 : comments #
Total comments: 2
Patch Set 7 : Add test, removed cleanup #
Total comments: 20
Patch Set 8 : comments and cleanup #
Messages
Total messages: 53 (14 generated)
Hey Michael! This is a release blocker bug fix. The upload data element reader has to be able to call 'Init' again, so I created a way to reset the reader. Let me know if you think I should do it differently. I verified that it fixes the problem. Daniel
https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... File storage/browser/blob/blob_reader.cc (right): https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... storage/browser/blob/blob_reader.cc:199: // We keep the map of file readers to be efficient. i think the readers maintain state like the current read position, and what if there's a pending io call when reset is called, you've snipped the completion callbacks to this class here, but a reader could still be busy internally another approach could be for the uploader to Kill() its blob reader and create an entirely one if it's being reinitted?
https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... File storage/browser/blob/blob_reader.cc (right): https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... storage/browser/blob/blob_reader.cc:199: // We keep the map of file readers to be efficient. On 2015/12/11 00:22:18, michaeln wrote: > i think the readers maintain state like the current read position, and what if > there's a pending io call when reset is called, you've snipped the completion > callbacks to this class here, but a reader could still be busy internally > > another approach could be for the uploader to Kill() its blob reader and create > an entirely one if it's being reinitted? It's real easy to miss resetting a particular member, more than one is not be reset in here currently. I also see the BlobReader ctor is protected and its not super easy to constuct a new one. Maybe a reader->is_dirty() method and a newreader = oldreader->Recreate() method to get everything reliably reinitted. https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... storage/browser/blob/blob_reader.cc:565: void BlobReader::SetFileReaderAtIndex(size_t index, The old impl completely ignored |index|, the rewrite uses it in one place but not the other.
On 2015/12/11 at 00:22:18, michaeln wrote: > https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... > File storage/browser/blob/blob_reader.cc (right): > > https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... > storage/browser/blob/blob_reader.cc:199: // We keep the map of file readers to be efficient. > i think the readers maintain state like the current read position, and what if there's a pending io call when reset is called, you've snipped the completion callbacks to this class here, but a reader could still be busy internally > > another approach could be for the uploader to Kill() its blob reader and create an entirely one if it's being reinitted? Yeah, I wasn't sure. When we re-create the reader, we would have to pass in the file system context and task runner, which we could save, but I was thinking it might be better just to make the reader resetable.
https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... File storage/browser/blob/blob_reader.cc (right): https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... storage/browser/blob/blob_reader.cc:199: // We keep the map of file readers to be efficient. On 2015/12/11 at 00:48:16, michaeln wrote: > On 2015/12/11 00:22:18, michaeln wrote: > > i think the readers maintain state like the current read position, and what if > > there's a pending io call when reset is called, you've snipped the completion > > callbacks to this class here, but a reader could still be busy internally > > > > another approach could be for the uploader to Kill() its blob reader and create > > an entirely one if it's being reinitted? > > > It's real easy to miss resetting a particular member, more than one is not be reset in here currently. I also see the BlobReader ctor is protected and its not super easy to constuct a new one. > > Maybe a reader->is_dirty() method and a newreader = oldreader->Recreate() method to get everything reliably reinitted. If we do that, then we'll need to make the old one invalid, as we have a couple scoped_ptr's we'd have to move. That seems more confusing than just having an init method. What if we did InitAndCalculateSize? https://codereview.chromium.org/1513783005/diff/1/storage/browser/blob/blob_r... storage/browser/blob/blob_reader.cc:565: void BlobReader::SetFileReaderAtIndex(size_t index, On 2015/12/11 at 00:48:16, michaeln wrote: > The old impl completely ignored |index|, the rewrite uses it in one place but not the other. Old was incorrect then.
dmurph@chromium.org changed reviewers: + mmenke@chromium.org
Switched to recreating the reader.
https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/bl... File storage/browser/blob/blob_reader.h (right): https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/bl... storage/browser/blob/blob_reader.h:69: // * This hsould only be called once per reader. stray key strike https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/up... File storage/browser/blob/upload_blob_element_reader.cc (right): https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/up... storage/browser/blob/upload_blob_element_reader.cc:29: handle_->CreateReader(file_system_context_.get(), file_runner_.get()); i think this is a nice improvement! i think you could always create a new reader_ here and never in the ctor
On 2015/12/11 at 02:17:01, michaeln wrote: > https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/bl... > File storage/browser/blob/blob_reader.h (right): > > https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/bl... > storage/browser/blob/blob_reader.h:69: // * This hsould only be called once per reader. > stray key strike > > https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/up... > File storage/browser/blob/upload_blob_element_reader.cc (right): > > https://codereview.chromium.org/1513783005/diff/60001/storage/browser/blob/up... > storage/browser/blob/upload_blob_element_reader.cc:29: handle_->CreateReader(file_system_context_.get(), file_runner_.get()); > i think this is a nice improvement! > > i think you could always create a new reader_ here and never in the ctor Done.
https://codereview.chromium.org/1513783005/diff/70001/storage/browser/blob/up... File storage/browser/blob/upload_blob_element_reader.cc (right): https://codereview.chromium.org/1513783005/diff/70001/storage/browser/blob/up... storage/browser/blob/upload_blob_element_reader.cc:25: if (reader_.get() == nullptr ||reader_->dirty()) { is there any reason to not always create a new reader when re-initialized? i'd vote to have slightly less and less complicated code and then the is_dirty() method wouldn't be needed in that case
On 2015/12/11 at 03:01:11, michaeln wrote: > https://codereview.chromium.org/1513783005/diff/70001/storage/browser/blob/up... > File storage/browser/blob/upload_blob_element_reader.cc (right): > > https://codereview.chromium.org/1513783005/diff/70001/storage/browser/blob/up... > storage/browser/blob/upload_blob_element_reader.cc:25: if (reader_.get() == nullptr ||reader_->dirty()) { > is there any reason to not always create a new reader when re-initialized? i'd vote to have slightly less and less complicated code > > and then the is_dirty() method wouldn't be needed in that case Sounds good.
Set 6 looks really good. Are the blob_reader changes necessary for correctness? They just look like cleanups to me. Given that we want to merge this into M48 (And I think M47 as well), think it's best to keep this CL as minimal as possible. The cleanups to look worthwhile, just not in a CL meant for merging. https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... File storage/browser/blob/upload_blob_element_reader.cc (right): https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... storage/browser/blob/upload_blob_element_reader.cc:20: file_runner_(file_task_runner) {} Should probably include base/single_thread_task_runner.h
On 2015/12/11 at 16:05:10, mmenke wrote: > Set 6 looks really good. > > Are the blob_reader changes necessary for correctness? They just look like cleanups to me. Given that we want to merge this into M48 (And I think M47 as well), think it's best to keep this CL as minimal as possible. The cleanups to look worthwhile, just not in a CL meant for merging. > > https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... > File storage/browser/blob/upload_blob_element_reader.cc (right): > > https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... > storage/browser/blob/upload_blob_element_reader.cc:20: file_runner_(file_task_runner) {} > Should probably include base/single_thread_task_runner.h They're sort of necessary for correctness due to not using the 'index' argument correctly in SetFileReaderAtIndex. I can undo the std::move code if you want.
On 2015/12/11 17:21:11, dmurph wrote: > On 2015/12/11 at 16:05:10, mmenke wrote: > > Set 6 looks really good. > > > > Are the blob_reader changes necessary for correctness? They just look like > cleanups to me. Given that we want to merge this into M48 (And I think M47 as > well), think it's best to keep this CL as minimal as possible. The cleanups to > look worthwhile, just not in a CL meant for merging. > > > > > https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... > > File storage/browser/blob/upload_blob_element_reader.cc (right): > > > > > https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... > > storage/browser/blob/upload_blob_element_reader.cc:20: > file_runner_(file_task_runner) {} > > Should probably include base/single_thread_task_runner.h > > They're sort of necessary for correctness due to not using the 'index' argument > correctly in SetFileReaderAtIndex. I can undo the std::move code if you want. Ah, right...I missed the current_item_index_ -> index change.
This looks great now. Do we want to split the cl? The index change srtictly isn't needed because all current callers pass current_index_ in as that input. The minimal one to merge: upload_blob_element_reader.cc .h upload_data_stream_builder.cc The other for cleanup: blob_reader.cc .h
On 2015/12/11 20:47:39, michaeln wrote: > This looks great now. > > Do we want to split the cl? The index change srtictly isn't needed because all > current callers pass current_index_ in as that input. > > The minimal one to merge: > upload_blob_element_reader.cc .h > upload_data_stream_builder.cc > > The other for cleanup: > blob_reader.cc .h I also think we want a test before landing.
On 2015/12/11 20:48:51, mmenke wrote: > On 2015/12/11 20:47:39, michaeln wrote: > > This looks great now. > > > > Do we want to split the cl? The index change srtictly isn't needed because all > > current callers pass current_index_ in as that input. > > > > The minimal one to merge: > > upload_blob_element_reader.cc .h > > upload_data_stream_builder.cc > > > > The other for cleanup: > > blob_reader.cc .h > > I also think we want a test before landing. We might want to put that in the 2nd cl? To avoid adding a new unittest .cc file to the project for the upload_blob_element_reader.cc which could complicate merging this and other subsequent cls. Something like the set in upload_file_element_reader_unittests.cc Merge the minimal one after we see bots running tests and canaries running browsers built from trunk.
On 2015/12/11 21:23:44, michaeln wrote: > On 2015/12/11 20:48:51, mmenke wrote: > > On 2015/12/11 20:47:39, michaeln wrote: > > > This looks great now. > > > > > > Do we want to split the cl? The index change srtictly isn't needed because > all > > > current callers pass current_index_ in as that input. > > > > > > The minimal one to merge: > > > upload_blob_element_reader.cc .h > > > upload_data_stream_builder.cc > > > > > > The other for cleanup: > > > blob_reader.cc .h > > > > I also think we want a test before landing. > > We might want to put that in the 2nd cl? To avoid adding a new unittest .cc file > to the project for the upload_blob_element_reader.cc which could complicate > merging this and other subsequent cls. Something like the set in > upload_file_element_reader_unittests.cc > > Merge the minimal one after we see bots running tests and canaries running > browsers built from trunk. I'm fine with not merging a test, but I think we want a landed test before asking for a merge.
I added the test to a class that's already there. How does this look? https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... File storage/browser/blob/upload_blob_element_reader.cc (right): https://codereview.chromium.org/1513783005/diff/90001/storage/browser/blob/up... storage/browser/blob/upload_blob_element_reader.cc:20: file_runner_(file_task_runner) {} On 2015/12/11 at 16:05:10, mmenke wrote: > Should probably include base/single_thread_task_runner.h Good catch, done.
This LGTM (Though michaeln should also review it, of course) https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:163: BlobStorageContext context; May want to call this blob_storage_context. Matches the test above, and the next argument to UploadDataStreamBuilder::Build is a storage::FileSystemContext, which makes the name context a bit unclear. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:171: request_body.get(), &context, NULL, nullptr https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:177: ASSERT_TRUE(r3); This doesn't seem to get us anything. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:183: int kBufferLength = 4; const https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:184: scoped_ptr<char[]> buffer(new char[kBufferLength]); scoped_array, or could just use IOBufferWithSize (Will need to cast to char* to read it). https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:190: EXPECT_EQ(static_cast<int>(kBufferLength), read_callback.GetResult(result)); kBufferLength is already an int. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:199: EXPECT_EQ(static_cast<int>(kBufferLength), read_callback.GetResult(result)); kBufferLength is already an int. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:200: EXPECT_EQ(0, std::memcmp(kBlobData.c_str(), buffer.get(), kBufferLength)); Should we read the entire body here, just in case?
lgtm 2 https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:154: ResetUploadStreamWithBlob) { does this not fit on one line? https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:177: ASSERT_TRUE(r3); On 2015/12/11 23:15:45, mmenke wrote: > This doesn't seem to get us anything. right, the local var is not used https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:184: scoped_ptr<char[]> buffer(new char[kBufferLength]); or you could put buffer on the stack, IOBufferWithSize sgtm2
https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... File content/browser/loader/upload_data_stream_builder_unittest.cc (right): https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:154: ResetUploadStreamWithBlob) { On 2015/12/11 at 23:21:33, michaeln wrote: > does this not fit on one line? Done. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:163: BlobStorageContext context; On 2015/12/11 at 23:15:45, mmenke wrote: > May want to call this blob_storage_context. Matches the test above, and the next argument to UploadDataStreamBuilder::Build is a storage::FileSystemContext, which makes the name context a bit unclear. Done. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:171: request_body.get(), &context, NULL, On 2015/12/11 at 23:15:45, mmenke wrote: > nullptr Done. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:177: ASSERT_TRUE(r3); On 2015/12/11 at 23:21:33, michaeln wrote: > On 2015/12/11 23:15:45, mmenke wrote: > > This doesn't seem to get us anything. > > right, the local var is not used REmoved. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:183: int kBufferLength = 4; On 2015/12/11 at 23:15:45, mmenke wrote: > const Done. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:184: scoped_ptr<char[]> buffer(new char[kBufferLength]); On 2015/12/11 at 23:21:33, michaeln wrote: > or you could put buffer on the stack, IOBufferWithSize sgtm2 IMPOSSIBLE (refcounted ;) Made it simpler though. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:190: EXPECT_EQ(static_cast<int>(kBufferLength), read_callback.GetResult(result)); On 2015/12/11 at 23:15:45, mmenke wrote: > kBufferLength is already an int. Done. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:199: EXPECT_EQ(static_cast<int>(kBufferLength), read_callback.GetResult(result)); On 2015/12/11 at 23:15:45, mmenke wrote: > kBufferLength is already an int. Done. https://codereview.chromium.org/1513783005/diff/110001/content/browser/loader... content/browser/loader/upload_data_stream_builder_unittest.cc:200: EXPECT_EQ(0, std::memcmp(kBlobData.c_str(), buffer.get(), kBufferLength)); On 2015/12/11 at 23:15:45, mmenke wrote: > Should we read the entire body here, just in case? Sure.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1513783005/#ps130001 (title: "comments and cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1513783005/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1513783005/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mmenke@chromium.org
On 2015/12/12 03:17:20, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) > linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux > (JOB_TIMED_OUT, no build URL) > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux > (JOB_TIMED_OUT, no build URL) > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no > build URL) > linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no > build URL) > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build > URL) CQing this...not sure if dmurph is OOO or what, I'll ask for a merge if it lands successfully.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1513783005/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1513783005/130001
On 2015/12/15 at 00:38:56, commit-bot wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1513783005/130001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1513783005/130001 Thanks, I missed that this didn't go in.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1513783005/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1513783005/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 ========== to ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed: https://crrev.com/622edffe2ccdaa487dab085a4689a9a11f33790c Cr-Commit-Position: refs/heads/master@{#365176} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/622edffe2ccdaa487dab085a4689a9a11f33790c Cr-Commit-Position: refs/heads/master@{#365176}
Message was sent while issue was closed.
Description was changed from ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed: https://crrev.com/622edffe2ccdaa487dab085a4689a9a11f33790c Cr-Commit-Position: refs/heads/master@{#365176} ========== to ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed: https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2 Cr-Commit-Position: refs/heads/master@{#365177} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2 Cr-Commit-Position: refs/heads/master@{#365177}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:130001) has been created in https://codereview.chromium.org/1529743002/ by yoichio@chromium.org. The reason for reverting is: CL was duplicated and compile failed. https://build.chromium.org/p/chromium.mac/builders/Mac%20GN/builds/22509 https://chromium.googlesource.com/chromium/src/+log/ebf7a6b19a7a468195a5606fb....
On 2015/12/15 at 06:38:19, yoichio wrote: > A revert of this CL (patchset #8 id:130001) has been created in https://codereview.chromium.org/1529743002/ by yoichio@chromium.org. > > The reason for reverting is: CL was duplicated and compile failed. > https://build.chromium.org/p/chromium.mac/builders/Mac%20GN/builds/22509 > https://chromium.googlesource.com/chromium/src/+log/ebf7a6b19a7a468195a5606fb.... How the hell did this get committed twice.
Description was changed from ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed: https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2 Cr-Commit-Position: refs/heads/master@{#365177} ========== to ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed-First-Time: https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2 Cr-Commit-Position-First-Time: refs/heads/master@{#365177} ==========
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1513783005/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1513783005/130001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1513783005/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1513783005/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed-First-Time: https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2 Cr-Commit-Position-First-Time: refs/heads/master@{#365177} ========== to ========== [Blob] Fix for resetting reader. R=michaeln BUG=567692 Committed-First-Time: https://crrev.com/ebf7a6b19a7a468195a5606fbd2bbd6d07a26ea2 Cr-Commit-Position-First-Time: refs/heads/master@{#365177} Committed: https://crrev.com/f01c1228c9cca9b4078443438dfa868f7b972ac2 Cr-Commit-Position: refs/heads/master@{#365316} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f01c1228c9cca9b4078443438dfa868f7b972ac2 Cr-Commit-Position: refs/heads/master@{#365316} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
