Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(121)

Issue 1310153003: Change WebURLLoaderMockFactory for future FOUC tests in blink. (Closed)

Created:
5 years, 3 months ago by esprehn
Modified:
5 years, 3 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change WebURLLoaderMockFactory for future FOUC tests in blink. In future FOUC unit tests blink will need to control the responses of network requests. This means blink may buffer the calls to didReceiveResponse, didReceiveData, didFail and didFinishLoading. To enable this we need to make it not an error to cancel() a load that doesn't exist. This is because ServeAsynchronousRequests will always do pending_loaders_.erase(loader); even if the request was actually buffered inside blink. That means later when blink replays the callbacks the request will already have been removed from pending_loaders_ when blink calls cancel() and then this would crash. I also updated ReadFile to support loading empty paths instead of needing to load an empty.txt file. This is because blink is going to simulate the response data in chunks and not actually load it from files. BUG=521692 R=dcheng@chromium.org Committed: https://crrev.com/c2a4a231d6d11c6cae4b1fac6ef3f56e31549827 Cr-Commit-Position: refs/heads/master@{#345748}

Patch Set 1 #

Total comments: 2

Patch Set 2 : dcheng review. #

Total comments: 1

Patch Set 3 : Comments + default #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -6 lines) Patch
M content/test/weburl_loader_mock_factory.h View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M content/test/weburl_loader_mock_factory.cc View 1 2 2 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 17 (7 generated)
esprehn
5 years, 3 months ago (2015-08-26 15:33:15 UTC) #2
dcheng
https://codereview.chromium.org/1310153003/diff/1/content/test/weburl_loader_mock_factory.cc File content/test/weburl_loader_mock_factory.cc (right): https://codereview.chromium.org/1310153003/diff/1/content/test/weburl_loader_mock_factory.cc#newcode111 content/test/weburl_loader_mock_factory.cc:111: LoaderToRequestMap::iterator iter = pending_loaders_.find(loader); How about just replacing the ...
5 years, 3 months ago (2015-08-26 15:38:18 UTC) #3
esprehn
https://codereview.chromium.org/1310153003/diff/1/content/test/weburl_loader_mock_factory.cc File content/test/weburl_loader_mock_factory.cc (right): https://codereview.chromium.org/1310153003/diff/1/content/test/weburl_loader_mock_factory.cc#newcode111 content/test/weburl_loader_mock_factory.cc:111: LoaderToRequestMap::iterator iter = pending_loaders_.find(loader); On 2015/08/26 at 15:38:18, dcheng ...
5 years, 3 months ago (2015-08-26 16:01:40 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310153003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310153003/20001
5 years, 3 months ago (2015-08-26 16:02:45 UTC) #6
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/103844)
5 years, 3 months ago (2015-08-26 16:24:41 UTC) #8
sky
LGTM https://codereview.chromium.org/1310153003/diff/20001/content/test/weburl_loader_mock_factory.cc File content/test/weburl_loader_mock_factory.cc (right): https://codereview.chromium.org/1310153003/diff/20001/content/test/weburl_loader_mock_factory.cc#newcode167 content/test/weburl_loader_mock_factory.cc:167: if (file_path.empty()) This is subtle and worth a ...
5 years, 3 months ago (2015-08-26 17:52:39 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310153003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310153003/40001
5 years, 3 months ago (2015-08-26 23:44:30 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310153003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310153003/40001
5 years, 3 months ago (2015-08-27 00:33:43 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 3 months ago (2015-08-27 00:39:34 UTC) #16
commit-bot: I haz the power
5 years, 3 months ago (2015-08-27 00:40:05 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/c2a4a231d6d11c6cae4b1fac6ef3f56e31549827
Cr-Commit-Position: refs/heads/master@{#345748}

Powered by Google App Engine
This is Rietveld 408576698