Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466883002:
Add TabLoaderTest (Closed)
|
Created:
4 years, 1 month ago by hajimehoshi Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd TabLoaderTest
This CL introduces TabLoaderTest to test TabLoader::OnMemoryStateChange
introduced at crrev.com/2370753002.
BUG=639700
TEST=unit_tests --gtest_filter=TabLoaderTest.*
Committed: https://crrev.com/eeaf192119af3bced58245ead864805f8eb2a5f2
Cr-Commit-Position: refs/heads/master@{#430584}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address on sky's review #
Total comments: 5
Patch Set 3 : (rebase) #Patch Set 4 : Address on reviews #
Total comments: 2
Patch Set 5 : Address on bashi's review; Fix compile errors on Android #Patch Set 6 : (rebase) #
Total comments: 2
Patch Set 7 : Fix compile error #Patch Set 8 : Fix include guard #Patch Set 9 : Disable tests on macOS #Patch Set 10 : Use ScopedFeatureList #
Depends on Patchset: Messages
Total messages: 59 (35 generated)
hajimehoshi@chromium.org changed reviewers: + bashi@chromium.org, chrisha@chromium.org, sky@chromium.org
PTAL
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM
https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/DEPS File chrome/browser/sessions/DEPS (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/DEP... chrome/browser/sessions/DEPS:9: "+content/browser/memory/memory_coordinator.h", My understanding is that chrome code should only use code in content/public. Can you make it possible to test memory_coordinator without needing private files? https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader.h (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader.h:72: static bool GetLoadingEnabledForTesting(); As the test is a friend, can't it call shared_tab_loader_->loading_enabled_ directly? That way you don't need to expose awkward test only functions like this. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader_unittest.cc (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:43: void SetUp() override { Prefix section with where overriding from (like you have on line 27). https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:46: coordinator_.reset(new content::MemoryCoordinatorImpl( MakeUnique where possible (see threads on chromium-dev). https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:77: }; DISALLOW... https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:85: loop.RunUntilIdle(); Why do you need the RunUntilIdle? https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:86: EXPECT_FALSE(TabLoader::GetLoadingEnabledForTesting()); It would also be good to verify TabLoader deletes itself too.
Thank you! https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/DEPS File chrome/browser/sessions/DEPS (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/DEP... chrome/browser/sessions/DEPS:9: "+content/browser/memory/memory_coordinator.h", On 2016/11/01 15:23:14, sky wrote: > My understanding is that chrome code should only use code in content/public. Can > you make it possible to test memory_coordinator without needing private files? Done. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader.h (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader.h:72: static bool GetLoadingEnabledForTesting(); On 2016/11/01 15:23:14, sky wrote: > As the test is a friend, can't it call shared_tab_loader_->loading_enabled_ > directly? That way you don't need to expose awkward test only functions like > this. Done. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader_unittest.cc (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:43: void SetUp() override { On 2016/11/01 15:23:14, sky wrote: > Prefix section with where overriding from (like you have on line 27). Done. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:46: coordinator_.reset(new content::MemoryCoordinatorImpl( On 2016/11/01 15:23:14, sky wrote: > MakeUnique where possible (see threads on chromium-dev). Done. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:77: }; On 2016/11/01 15:23:14, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:85: loop.RunUntilIdle(); On 2016/11/01 15:23:14, sky wrote: > Why do you need the RunUntilIdle? This is needed to ensure TabLoader::OnMemoryStateChange is called. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:86: EXPECT_FALSE(TabLoader::GetLoadingEnabledForTesting()); On 2016/11/01 15:23:14, sky wrote: > It would also be good to verify TabLoader deletes itself too. IIUC TabLoader::~TabLoader is not called until the process exits. https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... File content/public/browser/memory_coordinator_delegate.h (right): https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... content/public/browser/memory_coordinator_delegate.h:17: CONTENT_EXPORT void SetUpMemoryCoordinatorProxyForTesting(); chrisha: Is there a better place to put such functions?
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks for moving files to content/public. I have no other issues beyond the one below. https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... File chrome/browser/sessions/tab_loader_unittest.cc (right): https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... chrome/browser/sessions/tab_loader_unittest.cc:85: loop.RunUntilIdle(); On 2016/11/02 09:34:13, hajimehoshi wrote: > On 2016/11/01 15:23:14, sky wrote: > > Why do you need the RunUntilIdle? > > This is needed to ensure TabLoader::OnMemoryStateChange is called. Doesn't the line above this, SetCurrentMemoryStateForTesting(), trigger that call immediately?
On 2016/11/02 15:40:26, sky wrote: > Thanks for moving files to content/public. I have no other issues beyond the one > below. > > https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... > File chrome/browser/sessions/tab_loader_unittest.cc (right): > > https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... > chrome/browser/sessions/tab_loader_unittest.cc:85: loop.RunUntilIdle(); > On 2016/11/02 09:34:13, hajimehoshi wrote: > > On 2016/11/01 15:23:14, sky wrote: > > > Why do you need the RunUntilIdle? > > > > This is needed to ensure TabLoader::OnMemoryStateChange is called. > > Doesn't the line above this, SetCurrentMemoryStateForTesting(), trigger that > call immediately? And if SetCurrentMemoryStateForTesting is async, how come it can't be made immediate? Lastly, if there is a good reason for SetCurrentMemoryStateForTesting to be async please add a comment to the test.
https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... File content/public/browser/memory_coordinator_delegate.cc (right): https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... content/public/browser/memory_coordinator_delegate.cc:13: void EnableFeaturesForTesting() { Can we put these implementations in content/browser/memory ? https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... File content/public/browser/memory_coordinator_delegate.h (right): https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... content/public/browser/memory_coordinator_delegate.h:13: CONTENT_EXPORT void EnableFeaturesForTesting(); I'm a bit uneasy to put these functions under content/ directly. Maybe we should add a class which only has static methods like these. Also, I think we should add a new file for the class. MemoryCoordinatorDelegate is a class which injects logic from chrome/ to content/.
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... File content/public/browser/memory_coordinator_delegate.cc (right): https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... content/public/browser/memory_coordinator_delegate.cc:13: void EnableFeaturesForTesting() { On 2016/11/03 23:31:18, bashi1 wrote: > Can we put these implementations in content/browser/memory ? It looks like we can have implementations in content/public/test. https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... File content/public/browser/memory_coordinator_delegate.h (right): https://codereview.chromium.org/2466883002/diff/20001/content/public/browser/... content/public/browser/memory_coordinator_delegate.h:13: CONTENT_EXPORT void EnableFeaturesForTesting(); On 2016/11/03 23:31:18, bashi1 wrote: > I'm a bit uneasy to put these functions under content/ directly. Maybe we should > add a class which only has static methods like these. Also, I think we should > add a new file for the class. MemoryCoordinatorDelegate is a class which injects > logic from chrome/ to content/. I moved them to content/public/test/. There seems global functions there, so I keep them without a class.
On 2016/11/02 15:40:56, sky wrote: > On 2016/11/02 15:40:26, sky wrote: > > Thanks for moving files to content/public. I have no other issues beyond the > one > > below. > > > > > https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... > > File chrome/browser/sessions/tab_loader_unittest.cc (right): > > > > > https://codereview.chromium.org/2466883002/diff/1/chrome/browser/sessions/tab... > > chrome/browser/sessions/tab_loader_unittest.cc:85: loop.RunUntilIdle(); > > On 2016/11/02 09:34:13, hajimehoshi wrote: > > > On 2016/11/01 15:23:14, sky wrote: > > > > Why do you need the RunUntilIdle? > > > > > > This is needed to ensure TabLoader::OnMemoryStateChange is called. > > > > Doesn't the line above this, SetCurrentMemoryStateForTesting(), trigger that > > call immediately? > > And if SetCurrentMemoryStateForTesting is async, how come it can't be made > immediate? Lastly, if there is a good reason for SetCurrentMemoryStateForTesting > to be async please add a comment to the test. The reason is notifying uses ObserverListThreadsafe, which uses PostTask to notify. Added a comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2466883002/diff/60001/content/public/test/mem... File content/public/test/memory_coordinator_test_utils.h (right): https://codereview.chromium.org/2466883002/diff/60001/content/public/test/mem... content/public/test/memory_coordinator_test_utils.h:11: void EnableFeaturesForTesting(); content::EnableFeaturesForTesting() sounds a bit generic to me (that's why I suggested creating a class). Could you name it EnableMemoryCoordinatorForTesting() ?
Ok, thanks for the comment. LGTM
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! https://codereview.chromium.org/2466883002/diff/60001/content/public/test/mem... File content/public/test/memory_coordinator_test_utils.h (right): https://codereview.chromium.org/2466883002/diff/60001/content/public/test/mem... content/public/test/memory_coordinator_test_utils.h:11: void EnableFeaturesForTesting(); On 2016/11/07 14:11:30, bashi1 wrote: > content::EnableFeaturesForTesting() sounds a bit generic to me (that's why I > suggested creating a class). Could you name it > EnableMemoryCoordinatorForTesting() ? Oops, Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. Thanks for being patient with review comments! https://codereview.chromium.org/2466883002/diff/100001/content/public/test/me... File content/public/test/memory_coordinator_test_utils.h (right): https://codereview.chromium.org/2466883002/diff/100001/content/public/test/me... content/public/test/memory_coordinator_test_utils.h:5: #ifndef CONTENT_PUBLIC_BROWSER_MEMORY_COORDINATOR_TEST_UTILS_H_ nit: CONTENT_PUBLIC_TEST_MEMORY_COORDINATOR_TEST_UTILS_H_ ?
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sky@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2466883002/#ps130001 (title: "Fix include guard")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! https://codereview.chromium.org/2466883002/diff/100001/content/public/test/me... File content/public/test/memory_coordinator_test_utils.h (right): https://codereview.chromium.org/2466883002/diff/100001/content/public/test/me... content/public/test/memory_coordinator_test_utils.h:5: #ifndef CONTENT_PUBLIC_BROWSER_MEMORY_COORDINATOR_TEST_UTILS_H_ On 2016/11/08 06:19:21, bashi1 wrote: > nit: CONTENT_PUBLIC_TEST_MEMORY_COORDINATOR_TEST_UTILS_H_ ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> [3707:1287:1107/232955:1610377166186:FATAL:memory_monitor.cc(28)] Check failed: false. This seems NOTREACHED() is called in CreateMemoryMonitor(), which bashi@ worked on. bashi: Do you have any ideas to fix this?
On 2016/11/08 08:28:19, hajimehoshi wrote: > > [3707:1287:1107/232955:1610377166186:FATAL:memory_monitor.cc(28)] Check > failed: false. > > This seems NOTREACHED() is called in CreateMemoryMonitor(), which bashi@ worked > on. > > bashi: Do you have any ideas to fix this? Ah, you need to disable tests on mac when tests use memory coordinator because there is no MemoryMonitor on mac yet. e.g. https://cs.chromium.org/chromium/src/content/browser/memory/memory_coordinato...
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sky@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2466883002/#ps150001 (title: "Disable tests on macOS")
On 2016/11/08 08:36:53, bashi1 wrote: > On 2016/11/08 08:28:19, hajimehoshi wrote: > > > [3707:1287:1107/232955:1610377166186:FATAL:memory_monitor.cc(28)] Check > > failed: false. > > > > This seems NOTREACHED() is called in CreateMemoryMonitor(), which bashi@ > worked > > on. > > > > bashi: Do you have any ideas to fix this? > > Ah, you need to disable tests on mac when tests use memory coordinator because > there is no MemoryMonitor on mac yet. > > e.g. > https://cs.chromium.org/chromium/src/content/browser/memory/memory_coordinato... Thanks!
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
Failed to apply patch for
content/browser/memory/memory_coordinator_browsertest.cc:
While running git apply --index -3 -p1;
error: patch failed:
content/browser/memory/memory_coordinator_browsertest.cc:15
Falling back to three-way merge...
Applied patch to 'content/browser/memory/memory_coordinator_browsertest.cc'
with conflicts.
U content/browser/memory/memory_coordinator_browsertest.cc
Patch: content/browser/memory/memory_coordinator_browsertest.cc
Index: content/browser/memory/memory_coordinator_browsertest.cc
diff --git a/content/browser/memory/memory_coordinator_browsertest.cc
b/content/browser/memory/memory_coordinator_browsertest.cc
index
096163fa3aecdc676632489b571c28cc7d495874..297479c74fb7c49e89b40518fb3d38303e1b6814
100644
--- a/content/browser/memory/memory_coordinator_browsertest.cc
+++ b/content/browser/memory/memory_coordinator_browsertest.cc
@@ -7,6 +7,7 @@
#include "content/public/common/content_features.h"
#include "content/public/test/content_browser_test.h"
#include "content/public/test/content_browser_test_utils.h"
+#include "content/public/test/memory_coordinator_test_utils.h"
namespace content {
@@ -15,7 +16,7 @@ class MemoryCoordinatorTest : public ContentBrowserTest {
MemoryCoordinatorTest() {}
void SetUp() override {
- MemoryCoordinator::EnableFeaturesForTesting();
+ content::EnableMemoryCoordinatorFeaturesForTesting();
ContentBrowserTest::SetUp();
}
Patchset #10 (id:170001) has been deleted
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, sky@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2466883002/#ps190001 (title: "Use ScopedFeatureList")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Add TabLoaderTest This CL introduces TabLoaderTest to test TabLoader::OnMemoryStateChange introduced at crrev.com/2370753002. BUG=639700 TEST=unit_tests --gtest_filter=TabLoaderTest.* ========== to ========== Add TabLoaderTest This CL introduces TabLoaderTest to test TabLoader::OnMemoryStateChange introduced at crrev.com/2370753002. BUG=639700 TEST=unit_tests --gtest_filter=TabLoaderTest.* Committed: https://crrev.com/eeaf192119af3bced58245ead864805f8eb2a5f2 Cr-Commit-Position: refs/heads/master@{#430584} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/eeaf192119af3bced58245ead864805f8eb2a5f2 Cr-Commit-Position: refs/heads/master@{#430584} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
