Implement WebDataConsumerHandle.

Implement WebDataConsumerHandle.

This CL implements WebDataConsumerHandleImpl, a subclass of
WebDataConsumerHandle. This class will be used by fetch API with Streams.

BUG=418879
Committed: https://crrev.com/afb2ae608b792b38514f293bc0c887e4a61bfecb
Cr-Commit-Position: refs/heads/master@{#302006}

[yhirano, 2014-10-03 07:51:47]

yhirano@chromium.org changed reviewers:
+ tyoshino@chromium.org

[yhirano, 2014-10-03 07:51:48]

Blink side CL: https://codereview.chromium.org/626803002/

[tyoshino (SeeGerritForStatus), 2014-10-23 07:22:43]

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:7: #include <numeric>
limits?

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:9: #include "base/logging.h"
include src/mojo/public/c/system/types.h

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:22: // TODO(yhirano): We need
this variable definition to avoid a link error.
please write the action to take in addition to the reason since it's "TODO".

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:83: base::Unretained(this)));
just put this in the corresponding case block in the switch below?

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl.h (right):

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.h:22: size_t* readSize) override;
I remember I was asked not to specify override to implementation of Blink public
API. Does it apply to this file?

[yhirano, 2014-10-23 08:35:39]

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:7: #include <numeric>
On 2014/10/23 07:22:43, tyoshino wrote:
> limits?

Done.

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:9: #include "base/logging.h"
On 2014/10/23 07:22:43, tyoshino wrote:
> include src/mojo/public/c/system/types.h

Done.

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:22: // TODO(yhirano): We need
this variable definition to avoid a link error.
On 2014/10/23 07:22:43, tyoshino wrote:
> please write the action to take in addition to the reason since it's "TODO".

Done.

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.cc:83: base::Unretained(this)));
On 2014/10/23 07:22:43, tyoshino wrote:
> just put this in the corresponding case block in the switch below?

Done.

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl.h (right):

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.h:22: size_t* readSize) override;
On 2014/10/23 07:22:43, tyoshino wrote:
> I remember I was asked not to specify override to implementation of Blink
public
> API. Does it apply to this file?

Maybe. Should I make WebDataConsumerHandle's methods non-pure as well?

[tyoshino (SeeGerritForStatus), 2014-10-23 09:38:51]

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl.h (right):

https://codereview.chromium.org/620303002/diff/40001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl.h:22: size_t* readSize) override;
On 2014/10/23 08:35:39, yhirano wrote:
> On 2014/10/23 07:22:43, tyoshino wrote:
> > I remember I was asked not to specify override to implementation of Blink
> public
> > API. Does it apply to this file?
> 
> Maybe. Should I make WebDataConsumerHandle's methods non-pure as well?

Yeah. Please.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:57: const base::Closure&
stop)
how about on_stop or on_done?

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:79: rv =
handle_->read(&buffer, sizeof(buffer), kNone, &readSize);
how about checking rv here (if not kOk, break from the loop), and then insert
the read data when rv == kOk?

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:104: 
1 blank line

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:110: const
base::Closure& stop)
ditto

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:134: size_t read_size =
std::max(static_cast<size_t>(1), size);
1U?

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:137: rv =
handle_->endRead(read_size);
check rv?

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:246: Loop();
does this make the message loop busy?

[tyoshino (SeeGerritForStatus), 2014-10-23 11:18:18]

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:134: size_t read_size =
std::max(static_cast<size_t>(1), size);
On 2014/10/23 09:38:50, tyoshino wrote:
> 1U?

Never mind

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:246: Loop();
On 2014/10/23 09:38:51, tyoshino wrote:
> does this make the message loop busy?

Use RunLoop::Run() and QuitClosure()?

[yhirano, 2014-10-23 12:01:06]

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:57: const base::Closure&
stop)
On 2014/10/23 09:38:51, tyoshino wrote:
> how about on_stop or on_done?

Done.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:79: rv =
handle_->read(&buffer, sizeof(buffer), kNone, &readSize);
On 2014/10/23 09:38:50, tyoshino wrote:
> how about checking rv here (if not kOk, break from the loop), and then insert
> the read data when rv == kOk?

Done.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:104: 
On 2014/10/23 09:38:50, tyoshino wrote:
> 1 blank line

Done.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:110: const
base::Closure& stop)
On 2014/10/23 09:38:50, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:134: size_t read_size =
std::max(static_cast<size_t>(1), size);
On 2014/10/23 09:38:50, tyoshino wrote:
> 1U?

We still need static_cast<size_t> even if we use 1u.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:137: rv =
handle_->endRead(read_size);
On 2014/10/23 09:38:50, tyoshino wrote:
> check rv?

Done.

https://codereview.chromium.org/620303002/diff/60001/content/child/web_data_c...
content/child/web_data_consumer_handle_impl_unittest.cc:246: Loop();
On 2014/10/23 11:18:18, tyoshino wrote:
> On 2014/10/23 09:38:51, tyoshino wrote:
> > does this make the message loop busy?
> 
> Use RunLoop::Run() and QuitClosure()?

Done.

[tyoshino (SeeGerritForStatus), 2014-10-24 13:46:33]

lgtm

[yhirano, 2014-10-24 15:12:48]

yhirano@chromium.org changed reviewers:
+ nasko@chromium.org

[yhirano, 2014-10-24 15:12:49]

+nasko for OWNER review.

[nasko, 2014-10-24 18:13:43]

Mostly style issues.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:22: void* data, size_t size,
Flags flags, size_t* read_size) {
style: parameters need to be each on a new line

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:32: *read_size = rv ==
MOJO_RESULT_OK ? size_to_pass : 0;
Please don't conflate two operations on the same line.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:39: // TODO(yhirano): We need
this variable definition to avoid a link error.
What's the difference between this case and the one on line 24? One is a TODO
the other one is not.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:56: return rv == MOJO_RESULT_OK ?
Ok : UnexpectedError;
This line is convoluted. It should be either without "rv ==" or the tertiary
operator has to be on a line of its own.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl_unittest.cc:61:
main_message_loop_(main_message_loop), on_done_(on_done) {}
style: each member should be on a separate line

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl_unittest.cc:65: void ReadMore()
override {
style: this fits all on one line

[yhirano, 2014-10-27 01:11:52]

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:22: void* data, size_t size,
Flags flags, size_t* read_size) {
On 2014/10/24 18:13:43, nasko wrote:
> style: parameters need to be each on a new line

Done.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:32: *read_size = rv ==
MOJO_RESULT_OK ? size_to_pass : 0;
On 2014/10/24 18:13:43, nasko wrote:
> Please don't conflate two operations on the same line.

Done.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:39: // TODO(yhirano): We need
this variable definition to avoid a link error.
On 2014/10/24 18:13:43, nasko wrote:
> What's the difference between this case and the one on line 24? One is a TODO
> the other one is not.

It's not TODO. Fixed.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:56: return rv == MOJO_RESULT_OK ?
Ok : UnexpectedError;
On 2014/10/24 18:13:43, nasko wrote:
> This line is convoluted. It should be either without "rv ==" or the tertiary
> operator has to be on a line of its own.

Done.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl_unittest.cc:61:
main_message_loop_(main_message_loop), on_done_(on_done) {}
On 2014/10/24 18:13:43, nasko wrote:
> style: each member should be on a separate line

Done.

https://codereview.chromium.org/620303002/diff/120001/content/child/web_data_...
content/child/web_data_consumer_handle_impl_unittest.cc:65: void ReadMore()
override {
On 2014/10/24 18:13:43, nasko wrote:
> style: this fits all on one line

Done.

[nasko, 2014-10-27 16:53:41]

LGTM, provided the complex statements are broken into separate statements.

https://codereview.chromium.org/620303002/diff/140001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/140001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:36: rv == MOJO_RESULT_OK ?
size_to_pass : 0;
The initial comment was not about breaking the statement across two lines,
rather it was about breaking up into two separate statements.

[yhirano, 2014-10-28 01:54:47]

https://codereview.chromium.org/620303002/diff/140001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/140001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:36: rv == MOJO_RESULT_OK ?
size_to_pass : 0;
On 2014/10/27 16:53:41, nasko wrote:
> The initial comment was not about breaking the statement across two lines,
> rather it was about breaking up into two separate statements.

Done.

[nasko, 2014-10-28 04:10:19]

https://codereview.chromium.org/620303002/diff/140001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/140001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:36: rv == MOJO_RESULT_OK ?
size_to_pass : 0;
On 2014/10/28 01:54:47, yhirano wrote:
> On 2014/10/27 16:53:41, nasko wrote:
> > The initial comment was not about breaking the statement across two lines,
> > rather it was about breaking up into two separate statements.
> 
> Done.

The same pattern appears in the rest of the file and should be similarly fixed.
Sorry for the roundtrips, I assumed you will fix them all. I will flag the rest.

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:56: *available = rv == Ok ?
size_to_pass : 0;
Please split.

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:63: rv == MOJO_RESULT_OK ? Ok :
UnexpectedError;
Please split.

[yhirano, 2014-10-28 04:24:34]

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:63: rv == MOJO_RESULT_OK ? Ok :
UnexpectedError;
On 2014/10/28 04:10:18, nasko wrote:
> Please split.

I don't understand your intention here: Do you want to prohibit the conditional
operator?

[nasko, 2014-10-28 05:05:21]

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:63: rv == MOJO_RESULT_OK ? Ok :
UnexpectedError;
On 2014/10/28 04:24:34, yhirano wrote:
> On 2014/10/28 04:10:18, nasko wrote:
> > Please split.
> 
> I don't understand your intention here: Do you want to prohibit the
conditional
> operator?

Sorry, this one was my mistake.

[yhirano, 2014-10-28 05:21:02]

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
File content/child/web_data_consumer_handle_impl.cc (right):

https://codereview.chromium.org/620303002/diff/160001/content/child/web_data_...
content/child/web_data_consumer_handle_impl.cc:56: *available = rv == Ok ?
size_to_pass : 0;
On 2014/10/28 04:10:18, nasko wrote:
> Please split.

Done (I replaced Ok with MOJO_RESULT_OK because I noticed that rv was of type
MojoResult).

[nasko, 2014-10-28 14:00:17]

lgtm

[yhirano, 2014-10-28 14:31:07]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-10-28 14:32:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/620303002/180001

[commit-bot: I haz the power, 2014-10-28 14:39:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-28 14:39:32]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[yhirano, 2014-10-29 08:06:17]

nasko@: I updated the CL to avoid build failures. Can you take a look at it
again?

[nasko, 2014-10-29 18:01:56]

LGTM

[yhirano, 2014-10-30 00:26:36]

Thanks!

[yhirano, 2014-10-30 00:26:43]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-10-30 00:28:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/620303002/300001

[commit-bot: I haz the power, 2014-10-30 01:17:35]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2014-10-30 01:18:23]

Patchset 16 (id:??) landed as
https://crrev.com/afb2ae608b792b38514f293bc0c887e4a61bfecb
Cr-Commit-Position: refs/heads/master@{#302006}