Implement WebDataConsumerHandle::Reader.

Implement WebDataConsumerHandle::Reader.

We are changing WebDataConsumerHandle interface so as to use "Reader"
interface. This CL implements it in WebDataConsumerHandleImpl and
SharedMemoryDataConsumerHandle. The latter supports the deprecated functions
while the former doesn't.

BUG=480746
Committed: https://crrev.com/9fd47e6056fb77d728fc9411a3aca214baa28907
Cr-Commit-Position: refs/heads/master@{#334566}

[yhirano, 2015-06-05 10:29:36]

Patchset #1 (id:1) has been deleted

[yhirano, 2015-06-05 14:54:25]

Patchset #2 (id:40001) has been deleted

[yhirano, 2015-06-08 03:41:19]

Patchset #2 (id:60001) has been deleted

[yhirano, 2015-06-08 04:33:46]

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

[yhirano, 2015-06-08 04:35:14]

On 2015/06/08 04:33:57, yhirano wrote:

This CL depends on https://codereview.chromium.org/1149563007/ and
https://codereview.chromium.org/1150033004/.

[hiroshige, 2015-06-11 00:03:30]

CL title: including the class name of SharedMemoryDataConsumerHandle would be
helpful.

https://codereview.chromium.org/1164493008/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1164493008/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:286: context_->Clear();
Do not call Clear() here because a reader might be alive after Handle is
destructed here.

[yhirano, 2015-06-11 07:15:27]

Patchset #4 (id:120001) has been deleted

[yhirano, 2015-06-11 07:24:06]

> CL title: including the class name of SharedMemoryDataConsumerHandle would be
helpful.

This CL adds ReaderImpl to SharedMemoryDataConsumerHandle and
WebDataConsumerHandleImpl, and including both makes the title too long.

https://codereview.chromium.org/1164493008/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1164493008/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:286: context_->Clear();
On 2015/06/11 00:03:30, hiroshige wrote:
> Do not call Clear() here because a reader might be alive after Handle is
> destructed here.

Thanks, done.

[hiroshige, 2015-06-12 10:12:09]

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:122: // this task.
Please describe why we don't repost if |repost| is false here.
(I think this is because if NotifyInternal posted here is no on the runner's
thread on its execution, it means the previous reader is destructed and then a
new reader is created on another thread.)

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:442:
EXPECT_CALL(checkpoint, Call(2));
It would be better to test that didGetReadable() is called (or, add TODO).

https://codereview.chromium.org/1164493008/diff/160001/content/child/web_data...
File content/child/web_data_consumer_handle_impl.h (right):

https://codereview.chromium.org/1164493008/diff/160001/content/child/web_data...
content/child/web_data_consumer_handle_impl.h:50: Handle handle_;
Can we remove |handle_| (not used, actual |handle_| is in Context)?

[yhirano, 2015-06-15 10:23:00]

Patchset #6 (id:180001) has been deleted

[yhirano, 2015-06-15 10:23:23]

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:122: // this task.
On 2015/06/12 10:12:09, hiroshige wrote:
> Please describe why we don't repost if |repost| is false here.
> (I think this is because if NotifyInternal posted here is no on the runner's
> thread on its execution, it means the previous reader is destructed and then a
> new reader is created on another thread.)

Done.

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1164493008/diff/160001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:442:
EXPECT_CALL(checkpoint, Call(2));
On 2015/06/12 10:12:09, hiroshige wrote:
> It would be better to test that didGetReadable() is called (or, add TODO).

Done.

https://codereview.chromium.org/1164493008/diff/160001/content/child/web_data...
File content/child/web_data_consumer_handle_impl.h (right):

https://codereview.chromium.org/1164493008/diff/160001/content/child/web_data...
content/child/web_data_consumer_handle_impl.h:50: Handle handle_;
On 2015/06/12 10:12:09, hiroshige wrote:
> Can we remove |handle_| (not used, actual |handle_| is in Context)?

Done.

[hiroshige, 2015-06-15 10:37:49]

lgtm.

[yhirano, 2015-06-15 15:58:03]

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

[yhirano, 2015-06-15 15:58:04]

+kinuko for OWNER review.

[kinuko, 2015-06-16 00:33:33]

lgtm with some nits

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:114: // Do nothing.
nit: prefer early return over else-if's unless you have reason

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.h (right):

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:60: scoped_ptr<ReaderImpl>
obtainReaderImpl(Client* client);
ObtainReaderImpl?

(I see this corresponds to obtainReader() in the parent class, but if it's not
overriding it seems we have no reason not to follow guideline)

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:74: // This is an implicitely
acquired reader for deprecated APIs.
nit: implicitly

[yhirano, 2015-06-16 00:58:17]

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:114: // Do nothing.
On 2015/06/16 00:33:32, kinuko wrote:
> nit: prefer early return over else-if's unless you have reason

Done.

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.h (right):

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:60: scoped_ptr<ReaderImpl>
obtainReaderImpl(Client* client);
On 2015/06/16 00:33:33, kinuko wrote:
> ObtainReaderImpl?
> 
> (I see this corresponds to obtainReader() in the parent class, but if it's not
> overriding it seems we have no reason not to follow guideline)

Done.

https://codereview.chromium.org/1164493008/diff/200001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:74: // This is an implicitely
acquired reader for deprecated APIs.
On 2015/06/16 00:33:32, kinuko wrote:
> nit: implicitly

Done.

[yhirano, 2015-06-16 01:06:33]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-06-16 01:06:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1164493008/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-06-16 01:07:13]

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

[yhirano, 2015-06-16 02:23:27]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-06-16 02:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1164493008/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-06-16 02:23:37]

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

[yhirano, 2015-06-16 07:00:41]

Patchset #12 (id:320001) has been deleted

[yhirano, 2015-06-16 07:00:49]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-06-16 07:00:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1164493008/#ps300001
(title: " ")

[commit-bot: I haz the power, 2015-06-16 07:01:03]

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

[commit-bot: I haz the power, 2015-06-16 08:08:10]

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

[commit-bot: I haz the power, 2015-06-16 08:08:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2015-06-16 08:12:59]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-06-16 08:13:11]

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

[commit-bot: I haz the power, 2015-06-16 08:37:04]

Committed patchset #11 (id:300001)

[commit-bot: I haz the power, 2015-06-16 08:38:07]

Patchset 11 (id:??) landed as
https://crrev.com/9fd47e6056fb77d728fc9411a3aca214baa28907
Cr-Commit-Position: refs/heads/master@{#334566}