Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046203003:
[Fetch API] Introduce BytesConsumer (Closed)
Description[Fetch API] Introduce BytesConsumer
This CL introduces BytesConsumer interface which is expected to replace most of
WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer
is bound to a thread and an on-heap interface.
This CL also implements a bridge class from FetchDataConsumerHandle to
BytesConsumer. As it is not yet used, this CL doesn't change the actual
behavior.
BUG=610195
Committed: https://crrev.com/e14ef20c8ff5251390825f79e83f7db23e28dbf0
Cr-Commit-Position: refs/heads/master@{#408592}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : fix #Patch Set 3 : fix #
Total comments: 40
Patch Set 4 : done #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : fix #
Messages
Total messages: 47 (30 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not used anywhere, this CL doesn't change the actual behavior. BUG=610195 ========== to ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yes used, this CL doesn't change the actual behavior. BUG=610195 ==========
Description was changed from ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yes used, this CL doesn't change the actual behavior. BUG=610195 ========== to ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yet used, this CL doesn't change the actual behavior. BUG=610195 ==========
yhirano@chromium.org changed reviewers: + hiroshige@chromium.org
ping
On 2016/07/04 07:59:04, yhirano wrote: > ping ping
https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:55: class MODULES_EXPORT Client : public GarbageCollectedMixin { Are the callbacks to Client all async? https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:71: // closed by BytesConsumer's public methods such as beginRead. How about describing the concrete cases of when onClose() is called? e.g. "... gets errored by BytesConsumer's public methods, i.e. when read()/beginRead()/endRead() returns |Done|, drainAsBlobDataHandle()/drainAsFormData() returns non-null, or cancel() is called." "associated consumer gets closed by BytesConsumer's public methods" is not so immediately clear (e.g. I missed drainAsBlobDataHandle() cases when I wrote this comment at first). Is onClose() called when the consumer is already closed when setClient() is called? https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:77: // errored by BytesConsumer's public methods such as beginRead. Ditto. From the comments, drainAsBlobDataHandle()/drainAsFormData()/cancel() never causes the consumer to be errored, right? https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:90: virtual Result read(char* buffer, size_t /* size */, size_t* readSize); Do we allow |buffer| == nullptr when |size| == 0? Do we allow returning |Ok| with |size| == 0? Is |*readSize| always set to 0 when returned value is not |Ok|? I expect these are all true, and if so I want these to be clarified in the comment. https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:100: virtual Result beginRead(const char** buffer, size_t* available) = 0; Do we allow returning |Ok| with |*avaliable| == 0? Do we allow (or enforce?) |*buffer| == nullptr when |*available| == 0? Is |*available| always set to 0 when returned value is not |Ok|?
Patchset #2 (id:80001) has been deleted
As talked offline, I revised the design. https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:55: class MODULES_EXPORT Client : public GarbageCollectedMixin { On 2016/07/08 05:32:37, hiroshige wrote: > Are the callbacks to Client all async? I recovered "This function is not called when the state change is trigerred by public methods called by a user" statement to avoid sync notification. https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:71: // closed by BytesConsumer's public methods such as beginRead. On 2016/07/08 05:32:37, hiroshige wrote: > How about describing the concrete cases of when onClose() is called? > e.g. > "... gets errored by BytesConsumer's public methods, i.e. when > read()/beginRead()/endRead() returns |Done|, > drainAsBlobDataHandle()/drainAsFormData() returns non-null, or cancel() is > called." > > "associated consumer gets closed by BytesConsumer's public methods" is not so > immediately clear (e.g. I missed drainAsBlobDataHandle() cases when I wrote this > comment at first). > Hmm, I'm not sure if it helps users. And I think such statements are likely to be stale. > Is onClose() called when the consumer is already closed when setClient() is > called? No. https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:77: // errored by BytesConsumer's public methods such as beginRead. On 2016/07/08 05:32:37, hiroshige wrote: > Ditto. > > From the comments, drainAsBlobDataHandle()/drainAsFormData()/cancel() never > causes the consumer to be errored, right? ditto https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:90: virtual Result read(char* buffer, size_t /* size */, size_t* readSize); On 2016/07/08 05:32:37, hiroshige wrote: > Do we allow |buffer| == nullptr when |size| == 0? > Do we allow returning |Ok| with |size| == 0? > Is |*readSize| always set to 0 when returned value is not |Ok|? > > I expect these are all true, and if so I want these to be clarified in the > comment. true/false/true https://codereview.chromium.org/2046203003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:100: virtual Result beginRead(const char** buffer, size_t* available) = 0; On 2016/07/08 05:32:37, hiroshige wrote: > Do we allow returning |Ok| with |*avaliable| == 0? > Do we allow (or enforce?) |*buffer| == nullptr when |*available| == 0? > Is |*available| always set to 0 when returned value is not |Ok|? true/true/true
ping
On 2016/07/19 00:09:03, yhirano wrote: > ping ping
Sorry for delay. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:22: // the state becomes closed or errored, it will never change. |reading| means > |reading| Isn't it |readable|? https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:59: // This function is called when the state changes. This function can nit: "the state" is slightly ambiguous: PublicState or InternalState. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:107: // The type of the returned handle may not be meaningful. nit: "The type" here is slightly ambiguous: BlobDataHandle::type(), or the type of returned value (==BlobDataHandle). https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:34: close(); Calling close() here can cause onStateChange() call, but according to the comment ("This function is not called when the state change is trigerred by public methods called by a user"), onStateChange() shouldn't be called here. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:37: error(); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:58: close(); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:61: error(); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:72: error(); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:81: return m_reader->drainAsBlobDataHandle(FetchDataConsumerHandle::Reader::DisallowBlobWithInvalidSize); Shouldn't we call close() if the returned value is non-null? https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:83: return m_reader->drainAsBlobDataHandle(FetchDataConsumerHandle::Reader::AllowBlobWithInvalidSize); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:90: return m_reader->drainAsFormData(); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:139: case WebDataConsumerHandle::UnexpectedError: nit: Can we make these lines "default:", as done in BytesConsumerForDataConsumerHandle::read()? https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h:20: class MODULES_EXPORT BytesConsumerForDataConsumerHandle : public BytesConsumer, public WebDataConsumerHandle::Client { Add |final|. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h:37: Error getError() const override { return m_error; } How about inserting DCHECK(m_state == InternalState::Errored) to catch misuse of getError()? https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:189: TEST_F(BytesConsumerForDataConsumerHandleTest, ReadWhenReadable) Following tests do not have clients, but how about adding MockClient to all tests below, to make sure clients are not notified during read()/beginRead()/drainAsFormData() etc.? (as I commented in BytesConsumerForDataConsumerHandle::read(), I expect the client would be notified in read()/beginRead() in e.g. ReadWhenClosed test, which is not expected) https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:210: ASSERT_EQ(Result::ShouldWait, r); How about setting |size_t read = 42;| and adding EXPECT_EQ(0, read)? https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:222: ASSERT_EQ(Result::Done, r); ditto. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:315: EXPECT_EQ(blobDataHandle, consumer->drainAsBlobDataHandle(BytesConsumer::BlobSizePolicy::AllowBlobWithInvalidSize)); Please check that consumer->getPublicState() is Closed after this line. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:329: EXPECT_EQ(formData, consumer->drainAsFormData()); ditto.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumer.h (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:22: // the state becomes closed or errored, it will never change. |reading| means On 2016/07/26 06:48:11, hiroshige wrote: > > |reading| > Isn't it |readable|? Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:59: // This function is called when the state changes. This function can On 2016/07/26 06:48:11, hiroshige wrote: > nit: "the state" is slightly ambiguous: PublicState or InternalState. The master definition is at (L21-L22). "state" there is an abstract idea, and both InternalState and PublicState are concrete enums in C++. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumer.h:107: // The type of the returned handle may not be meaningful. On 2016/07/26 06:48:11, hiroshige wrote: > nit: "The type" here is slightly ambiguous: BlobDataHandle::type(), or the type > of returned value (==BlobDataHandle). Is it clearer now? https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:34: close(); On 2016/07/26 06:48:12, hiroshige wrote: > Calling close() here can cause onStateChange() call, but according to the > comment ("This function is not called when the state change is trigerred by > public methods called by a user"), onStateChange() shouldn't be called here. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:37: error(); On 2016/07/26 06:48:12, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:58: close(); On 2016/07/26 06:48:11, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:61: error(); On 2016/07/26 06:48:12, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:72: error(); On 2016/07/26 06:48:12, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:81: return m_reader->drainAsBlobDataHandle(FetchDataConsumerHandle::Reader::DisallowBlobWithInvalidSize); On 2016/07/26 06:48:12, hiroshige wrote: > Shouldn't we call close() if the returned value is non-null? Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:83: return m_reader->drainAsBlobDataHandle(FetchDataConsumerHandle::Reader::AllowBlobWithInvalidSize); On 2016/07/26 06:48:11, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:90: return m_reader->drainAsFormData(); On 2016/07/26 06:48:12, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:139: case WebDataConsumerHandle::UnexpectedError: On 2016/07/26 06:48:12, hiroshige wrote: > nit: Can we make these lines "default:", as done in > BytesConsumerForDataConsumerHandle::read()? My understanding is "default" is not preferred because without it we can rely on the compiler warning when we add a value to the enum. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h:20: class MODULES_EXPORT BytesConsumerForDataConsumerHandle : public BytesConsumer, public WebDataConsumerHandle::Client { On 2016/07/26 06:48:12, hiroshige wrote: > Add |final|. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.h:37: Error getError() const override { return m_error; } On 2016/07/26 06:48:12, hiroshige wrote: > How about inserting DCHECK(m_state == InternalState::Errored) to catch misuse of > getError()? Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:189: TEST_F(BytesConsumerForDataConsumerHandleTest, ReadWhenReadable) On 2016/07/26 06:48:12, hiroshige wrote: > Following tests do not have clients, but how about adding MockClient to all > tests below, to make sure clients are not notified during > read()/beginRead()/drainAsFormData() etc.? > (as I commented in BytesConsumerForDataConsumerHandle::read(), I expect the > client would be notified in read()/beginRead() in e.g. ReadWhenClosed test, > which is not expected) Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:210: ASSERT_EQ(Result::ShouldWait, r); On 2016/07/26 06:48:12, hiroshige wrote: > How about setting |size_t read = 42;| and adding EXPECT_EQ(0, read)? Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:222: ASSERT_EQ(Result::Done, r); On 2016/07/26 06:48:12, hiroshige wrote: > ditto. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:315: EXPECT_EQ(blobDataHandle, consumer->drainAsBlobDataHandle(BytesConsumer::BlobSizePolicy::AllowBlobWithInvalidSize)); On 2016/07/26 06:48:12, hiroshige wrote: > Please check that consumer->getPublicState() is Closed after this line. Done. https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandleTest.cpp:329: EXPECT_EQ(formData, consumer->drainAsFormData()); On 2016/07/26 06:48:12, hiroshige wrote: > ditto. Done.
https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:139: case WebDataConsumerHandle::UnexpectedError: On 2016/07/27 13:19:52, yhirano wrote: > On 2016/07/26 06:48:12, hiroshige wrote: > > nit: Can we make these lines "default:", as done in > > BytesConsumerForDataConsumerHandle::read()? > > My understanding is "default" is not preferred because without it we can rely on > the compiler warning when we add a value to the enum. Agree. Then can we turn |default| in BytesConsumerForDataConsumerHandle::read()/beginRead() into |case|s?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp (right): https://codereview.chromium.org/2046203003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/BytesConsumerForDataConsumerHandle.cpp:139: case WebDataConsumerHandle::UnexpectedError: On 2016/07/28 06:40:12, hiroshige wrote: > On 2016/07/27 13:19:52, yhirano wrote: > > On 2016/07/26 06:48:12, hiroshige wrote: > > > nit: Can we make these lines "default:", as done in > > > BytesConsumerForDataConsumerHandle::read()? > > > > My understanding is "default" is not preferred because without it we can rely > on > > the compiler warning when we add a value to the enum. > > Agree. Then can we turn |default| in > BytesConsumerForDataConsumerHandle::read()/beginRead() into |case|s? Thanks, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The code looks good but bots are red.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/28 09:04:13, hiroshige wrote: > The code looks good but bots are red. Fixed.
LGTM. (It's unfortunate that we need NOTREACHED() after the switch statements though)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yet used, this CL doesn't change the actual behavior. BUG=610195 ========== to ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yet used, this CL doesn't change the actual behavior. BUG=610195 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yet used, this CL doesn't change the actual behavior. BUG=610195 ========== to ========== [Fetch API] Introduce BytesConsumer This CL introduces BytesConsumer interface which is expected to replace most of WebDataConsumerHandle subclasses. Unlike WebDataConsumerHandle, BytesConsumer is bound to a thread and an on-heap interface. This CL also implements a bridge class from FetchDataConsumerHandle to BytesConsumer. As it is not yet used, this CL doesn't change the actual behavior. BUG=610195 Committed: https://crrev.com/e14ef20c8ff5251390825f79e83f7db23e28dbf0 Cr-Commit-Position: refs/heads/master@{#408592} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e14ef20c8ff5251390825f79e83f7db23e28dbf0 Cr-Commit-Position: refs/heads/master@{#408592} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
