Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(130)

Issue 1179393007: [2a] Implement FetchDataLoader (Closed)

Created:
4 years, 10 months ago by hiroshige
Modified:
4 years, 10 months ago
Reviewers:
yhirano
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Implement FetchDataLoader This class is intended to replace BlobCreator and FileReader currently used in fetch/modules for data loading. BUG=480746 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197459

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Build break fix. #

Patch Set 4 : Move client to start(). #

Total comments: 24

Patch Set 5 : Reflect comments (except for making GarbageCollected). #

Total comments: 6

Patch Set 6 : #

Total comments: 1

Patch Set 7 : Add tests. #

Patch Set 8 : #

Total comments: 8

Patch Set 9 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+805 lines, -0 lines) Patch
A Source/modules/fetch/FetchDataLoader.h View 1 2 3 4 5 6 7 8 1 chunk +70 lines, -0 lines 0 comments Download
A Source/modules/fetch/FetchDataLoader.cpp View 1 2 3 4 5 1 chunk +232 lines, -0 lines 0 comments Download
A Source/modules/fetch/FetchDataLoaderTest.cpp View 1 2 3 4 5 6 7 8 1 chunk +500 lines, -0 lines 0 comments Download
M Source/modules/modules.gypi View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (3 generated)
hiroshige
PTAL.
4 years, 10 months ago (2015-06-16 12:00:00 UTC) #2
hiroshige
Depending on FetchDataConsumerHandle in https://codereview.chromium.org/1176243004/
4 years, 10 months ago (2015-06-16 12:53:53 UTC) #3
yhirano
Can we have unit tests? https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/FetchDataLoader.cpp File Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/FetchDataLoader.cpp#newcode6 Source/modules/fetch/FetchDataLoader.cpp:6: #include "modules/fetch/FetchDataLoader.h" +wtf/text/WTFString.h https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/FetchDataLoader.cpp#newcode16 ...
4 years, 10 months ago (2015-06-17 05:18:54 UTC) #4
hiroshige
I'll respond about unit tests and GC-related things in a later CL. https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/FetchDataLoader.cpp File Source/modules/fetch/FetchDataLoader.cpp ...
4 years, 10 months ago (2015-06-17 08:48:55 UTC) #5
yhirano
https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/FetchDataLoader.cpp File Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/1179393007/diff/80001/Source/modules/fetch/FetchDataLoader.cpp#newcode33 Source/modules/fetch/FetchDataLoader.cpp:33: if (blobHandle && blobHandle->size() != kuint64max) { Hmm, If ...
4 years, 10 months ago (2015-06-17 09:27:58 UTC) #6
hiroshige
(except for unit tests) https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/FetchDataLoader.h File Source/modules/fetch/FetchDataLoader.h (right): https://codereview.chromium.org/1179393007/diff/60001/Source/modules/fetch/FetchDataLoader.h#newcode36 Source/modules/fetch/FetchDataLoader.h:36: class FetchDataLoader { On 2015/06/17 ...
4 years, 10 months ago (2015-06-17 18:27:29 UTC) #8
yhirano
https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/FetchDataLoader.h File Source/modules/fetch/FetchDataLoader.h (right): https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/FetchDataLoader.h#newcode60 Source/modules/fetch/FetchDataLoader.h:60: virtual void start(FetchDataConsumerHandle* /* handle */, Client*) = 0; ...
4 years, 10 months ago (2015-06-18 11:05:06 UTC) #9
hiroshige
On 2015/06/18 11:05:06, yhirano wrote: > https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/FetchDataLoader.h > File Source/modules/fetch/FetchDataLoader.h (right): > > https://codereview.chromium.org/1179393007/diff/120001/Source/modules/fetch/FetchDataLoader.h#newcode60 > ...
4 years, 10 months ago (2015-06-18 11:48:02 UTC) #10
hiroshige
Added unit tests.
4 years, 10 months ago (2015-06-18 18:19:53 UTC) #11
yhirano
https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/FetchDataLoader.h File Source/modules/fetch/FetchDataLoader.h (right): https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/FetchDataLoader.h#newcode27 Source/modules/fetch/FetchDataLoader.h:27: class FetchDataLoader : public GarbageCollectedFinalized<FetchDataLoader> { Is it worth ...
4 years, 10 months ago (2015-06-19 06:19:30 UTC) #12
hiroshige
Also added a test for loading a string with null bytes. https://codereview.chromium.org/1179393007/diff/160001/Source/modules/fetch/FetchDataLoader.h File Source/modules/fetch/FetchDataLoader.h (right): ...
4 years, 10 months ago (2015-06-19 07:27:57 UTC) #13
yhirano
LGTM!
4 years, 10 months ago (2015-06-19 07:33:53 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1179393007/180001
4 years, 10 months ago (2015-06-19 08:00:49 UTC) #16
commit-bot: I haz the power
4 years, 10 months ago (2015-06-19 09:20:47 UTC) #17
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197459

Powered by Google App Engine
This is Rietveld 408576698