Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1991153002:
Reload cache control test: add more tests (Closed)
|
Created:
4 years, 7 months ago by Takashi Toyoshima Modified:
4 years ago Reviewers:
mmenke CC:
chromium-reviews, loading-reviews_chromium.org, darin-cc_chromium.org, jam, Charlie Reis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReload cache control test: add more tests
Add new test case for the same page navigation.
Also, add additional checks with DevTool shown/closed
for each test case.
BUG=670232, 671545
TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*'
TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation
Committed: https://crrev.com/a27638a0c9a2677e92d024a733b6c4884d48f0e3
Cr-Commit-Position: refs/heads/master@{#436559}
Patch Set 1 #Patch Set 2 : update #
Total comments: 8
Patch Set 3 : review #5 #Patch Set 4 : disable NavigateToSame if browser-side navigation is enabled #Messages
Total messages: 31 (15 generated)
Description was changed from ========== Reload cache control test: add same page navigation cases The second navigation should revalidate only the main resource, but follows protocol for other subresources. The third navigation should be same, but it behaves as a usual reload for some unknown reasons in the test. BUG=n/a ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=n/a ==========
toyoshim@chromium.org changed reviewers: + mmenke@chromium.org
Description was changed from ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=n/a ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232 ==========
Can you take a look?
https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... File content/browser/loader/reload_cache_control_browsertest.cc (right): https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:59: std::vector<RequestLog> request_log_; While you're here, mind including string and vector? https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:72: DISALLOW_COPY_AND_ASSIGN(ReloadCacheControlBrowserTest); And macros.h https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:90: EXPECT_EQ(kNoCacheControl, request_log_[3].cache_control); These tests don't seem to be threadsafe - there's no lock around request_log_, which is accessed on multiple threads. I guess there's an implicit memory barrier when the embedded test server writes to the socket, so as long as there are no unexpected requests (Like for favicons), this should be safe, but should we have one out of paranoia? https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:159: EXPECT_EQ(kNoCacheControl, request_log_[1].cache_control); I assume we have plenty of tests for has navigations (i.e. that they don't trigger a reload).
One other question...Does this really belong in browser/loader/? Doesn't seem like this is really testing RDH/RL or the ResourceHandler chain.
These tests are for end to end tests between Blink and content::NavigationController actually. So, could be in browser/frame_host, but I placed here because this is something that loading team want to care.
Description was changed from ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232 ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' ==========
Thank you for many good catches. https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... File content/browser/loader/reload_cache_control_browsertest.cc (right): https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:59: std::vector<RequestLog> request_log_; On 2016/12/02 20:16:49, mmenke wrote: > While you're here, mind including string and vector? Done. https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:72: DISALLOW_COPY_AND_ASSIGN(ReloadCacheControlBrowserTest); On 2016/12/02 20:16:49, mmenke wrote: > And macros.h Done. https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:90: EXPECT_EQ(kNoCacheControl, request_log_[3].cache_control); Thanks. I introduced request_log_lock_. https://codereview.chromium.org/1991153002/diff/20001/content/browser/loader/... content/browser/loader/reload_cache_control_browsertest.cc:159: EXPECT_EQ(kNoCacheControl, request_log_[1].cache_control); Yes, PlzNavigate team have many navigation related tests. So, here I just focus on testing the same page navigation as a kind of reload variants.
On 2016/12/05 06:09:03, toyoshim wrote: > These tests are for end to end tests between Blink and > content::NavigationController actually. > So, could be in browser/frame_host, but I placed here because this is something > that loading team want to care. Yea, unclear where the best place for this sort of integration-y test is. FrameHost does seem like a better location to me, but doesn't seem clear cut. Anyhow, LGTM, regardless of where you land it.
So, at this point, let me land this patch as is, keeping this file here. I will revisit this problem soon in the context of platform/loading. We just started discussing where we want to gather loading related layout/unit tests in Blink. Probably we need to discuss it for content too.
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hum... the test is build with two configurations, and one on Linux seems to fail. That says something is wrong with PlzNavigate.
OK, the content_browsertests binary run with two configurations on Linux. In the second configuration, browser-side navigation being enabaled, the NavigateToSame test fails. I think something is still wrong in the mode. So, I filed a bug, and disable it for now.
Description was changed from ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation ==========
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1991153002/#ps60001 (title: "disable NavigateToSame if browser-side navigation is enabled")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232, 671545 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation ==========
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481017557190070,
"parent_rev": "074d1336770939b947c70e211f9c426be369e40f", "commit_rev":
"396db82dcdebd0dcd8029110c448e67b169b6cf2"}
Message was sent while issue was closed.
Description was changed from ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232, 671545 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232, 671545 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232, 671545 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation ========== to ========== Reload cache control test: add more tests Add new test case for the same page navigation. Also, add additional checks with DevTool shown/closed for each test case. BUG=670232, 671545 TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' TEST=./out/Release/content_browsertests --gtest_filter='Reload*.*' --enable-browser-side-navigation Committed: https://crrev.com/a27638a0c9a2677e92d024a733b6c4884d48f0e3 Cr-Commit-Position: refs/heads/master@{#436559} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a27638a0c9a2677e92d024a733b6c4884d48f0e3 Cr-Commit-Position: refs/heads/master@{#436559} |
