Add tests to FetchBlobDataConsumerHandle

Add tests to FetchBlobDataConsumerHandle

BUG=480746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199014

[yhirano, 2015-07-08 07:19:27]

Patchset #1 (id:1) has been deleted

[yhirano, 2015-07-08 07:19:35]

Patchset #1 (id:20001) has been deleted

[yhirano, 2015-07-08 07:19:42]

Patchset #1 (id:40001) has been deleted

[yhirano, 2015-07-08 07:19:50]

Patchset #1 (id:60001) has been deleted

[yhirano, 2015-07-08 07:19:58]

Patchset #1 (id:80001) has been deleted

[yhirano, 2015-07-08 07:20:04]

Patchset #1 (id:100001) has been deleted

[yhirano, 2015-07-08 11:39:33]

Patchset #5 (id:200001) has been deleted

[yhirano, 2015-07-08 11:39:38]

Patchset #4 (id:180001) has been deleted

[yhirano, 2015-07-08 11:39:42]

Patchset #3 (id:160001) has been deleted

[yhirano, 2015-07-08 11:39:46]

Patchset #2 (id:140001) has been deleted

[yhirano, 2015-07-08 11:39:50]

Patchset #1 (id:120001) has been deleted

[yhirano, 2015-07-09 02:19:31]

Patchset #2 (id:240001) has been deleted

[yhirano, 2015-07-09 02:19:37]

Patchset #1 (id:220001) has been deleted

[yhirano, 2015-07-09 04:26:02]

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

[hiroshige, 2015-07-15 11:27:55]

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right):

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:99:
OwnPtr<DummyPageHolder> m_dummyPageHolder;
|m_dummyPageHolder| can be private.

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:237:
handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
optional: can we read() here and the following runPendingTasks()? Because
HandleReader should work without these.
(unless writing the test becomes harder if we don't explicitly start the loader
by the read() here)

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:274:
handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
ditto.

[yhirano, 2015-07-15 13:07:08]

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right):

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:237:
handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
On 2015/07/15 11:27:54, hiroshige wrote:
> optional: can we read() here and the following runPendingTasks()? Because
> HandleReader should work without these.
> (unless writing the test becomes harder if we don't explicitly start the
loader
> by the read() here)

Sorry, I'm not sure what you propose: can you restate?

[hiroshige, 2015-07-16 05:01:36]

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right):

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:237:
handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
On 2015/07/15 13:07:08, yhirano wrote:
> On 2015/07/15 11:27:54, hiroshige wrote:
> > optional: can we read() here and the following runPendingTasks()? Because
> > HandleReader should work without these.
> > (unless writing the test becomes harder if we don't explicitly start the
> loader
> > by the read() here)
> 
> Sorry, I'm not sure what you propose: can you restate?

Er, I originally thought as the following but changed my mind:
in this test we call
  handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
explicitly only to start a loader in L237 but this seems artificial for me.
I thought the code L242-L245 would work without such call to read() because the
loader will be started by read() etc. called from HandleReader, except for
factory->create() and client->didReceiveResponse() are called with different
timings.
However, I noticed controlling the timings would be trickier if I remove read()
and let HandleReader to trigger loader starting, so the current code is now fine
with me.

[yhirano, 2015-07-16 05:29:11]

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
File Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp (right):

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:99:
OwnPtr<DummyPageHolder> m_dummyPageHolder;
On 2015/07/15 11:27:55, hiroshige wrote:
> |m_dummyPageHolder| can be private.

Done.

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:237:
handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
On 2015/07/16 05:01:36, hiroshige wrote:
> On 2015/07/15 13:07:08, yhirano wrote:
> > On 2015/07/15 11:27:54, hiroshige wrote:
> > > optional: can we read() here and the following runPendingTasks()? Because
> > > HandleReader should work without these.
> > > (unless writing the test becomes harder if we don't explicitly start the
> > loader
> > > by the read() here)
> > 
> > Sorry, I'm not sure what you propose: can you restate?
> 
> Er, I originally thought as the following but changed my mind:
> in this test we call
>   handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
> explicitly only to start a loader in L237 but this seems artificial for me.
> I thought the code L242-L245 would work without such call to read() because
the
> loader will be started by read() etc. called from HandleReader, except for
> factory->create() and client->didReceiveResponse() are called with different
> timings.
> However, I noticed controlling the timings would be trickier if I remove
read()
> and let HandleReader to trigger loader starting, so the current code is now
fine
> with me.

Acknowledged.

https://codereview.chromium.org/1199043004/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchBlobDataConsumerHandleTest.cpp:274:
handle->obtainReader(nullptr)->read(nullptr, 0, kNone, &size);
On 2015/07/15 11:27:55, hiroshige wrote:
> ditto.

Acknowledged.

[hiroshige, 2015-07-16 05:30:23]

lgtm, thanks!

[yhirano, 2015-07-16 05:31:46]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-07-16 05:32:02]

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

[commit-bot: I haz the power, 2015-07-16 07:47:44]

Committed patchset #2 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199014