Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592363002:
FontResourceTest: call unregisterURL() respectively (Closed)
|
Created:
4 years ago by Takashi Toyoshima Modified:
4 years ago CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFontResourceTest: call unregisterURL() respectively
FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators
test calls registerURL(), but unregisterURL() isn't called for the test.
This fact makes following CacheAwareFontLoading test fails due to a check
failure because WebURLLoaderMockFactory does not allow us to call
registerURL() for the same URL before calling unregisterURL() for the
same URL. Since the current Platform instance and WebURLLoaderMockFactory
outlive each test instance, the first test affects the second test.
This error is recovered by a retry, but all should pass at the first run.
This fix is still leaving room for discussion to make it work safely, but
this is what we are doing for WebURLLoaderMockFactory in other places today.
BUG=n/a
TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*'
Committed: https://crrev.com/58afbd36cf5ee25c2126c47f4bc72fea04d0704f
Cr-Commit-Position: refs/heads/master@{#440362}
Patch Set 1 #
Total comments: 4
Patch Set 2 : done #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This fix is still leaving room discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a ========== to ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a ==========
toyoshim@chromium.org changed reviewers: + kinuko@chromium.org, yhirano@chromium.org
Description was changed from ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a ========== to ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' ==========
ptal
Description was changed from ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' ========== to ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' ==========
Description was changed from ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is waht we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' ========== to ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is what we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' ==========
lgtm
lgtm https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/FontResourceTest.cpp (right): https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/FontResourceTest.cpp:87: Platform::current()->getURLLoaderMockFactory()->unregisterURL(url); Do we rather want to have unregisterAllURLs() in FontResourceTest::TearDown() not to leak URLs?
https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h (right): https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h:49: // |response| for the same |url| for another test. These are WebURLLoaderMockFactory overrides, we may not really want to have comments between methods. Add this phrase to WebURLLoaderMockFactory.h and forward readers to WebURLLoaderMockFactory.h to see how it can be used?
https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/FontResourceTest.cpp (right): https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/FontResourceTest.cpp:87: Platform::current()->getURLLoaderMockFactory()->unregisterURL(url); On 2016/12/22 06:37:43, kinuko wrote: > Do we rather want to have unregisterAllURLs() in FontResourceTest::TearDown() > not to leak URLs? Done. https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h (right): https://codereview.chromium.org/2592363002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h:49: // |response| for the same |url| for another test. On 2016/12/22 06:39:26, kinuko wrote: > These are WebURLLoaderMockFactory overrides, we may not really want to have > comments between methods. > > Add this phrase to WebURLLoaderMockFactory.h and forward readers to > WebURLLoaderMockFactory.h to see how it can be used? Done.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2592363002/#ps20001 (title: "done")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482390211800490,
"parent_rev": "7bc0ba5ac76ccac93a3c79afa7bd6ae263ea1075", "commit_rev":
"fb6170cc10c65f5da1dff36be8f345da26b84fc6"}
Message was sent while issue was closed.
Description was changed from ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is what we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' ========== to ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is what we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' Review-Url: https://codereview.chromium.org/2592363002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is what we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' Review-Url: https://codereview.chromium.org/2592363002 ========== to ========== FontResourceTest: call unregisterURL() respectively FontResourceTest.ResourceFetcherRevalidateDeferedResourceFromTwoInitiators test calls registerURL(), but unregisterURL() isn't called for the test. This fact makes following CacheAwareFontLoading test fails due to a check failure because WebURLLoaderMockFactory does not allow us to call registerURL() for the same URL before calling unregisterURL() for the same URL. Since the current Platform instance and WebURLLoaderMockFactory outlive each test instance, the first test affects the second test. This error is recovered by a retry, but all should pass at the first run. This fix is still leaving room for discussion to make it work safely, but this is what we are doing for WebURLLoaderMockFactory in other places today. BUG=n/a TEST=webkit_unit_tests --gtset_filter='FontResourceTest.*' Committed: https://crrev.com/58afbd36cf5ee25c2126c47f4bc72fea04d0704f Cr-Commit-Position: refs/heads/master@{#440362} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/58afbd36cf5ee25c2126c47f4bc72fea04d0704f Cr-Commit-Position: refs/heads/master@{#440362} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
