Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1231593002:
Add some browser tests for the cast system tray. (Closed)
DescriptionAdd some browser tests for the cast system tray.
This CL depends on https://codereview.chromium.org/1224643008/ and https://codereview.chromium.org/1218653006/.
BUG=497343
Patch Set 1 #
Total comments: 43
Patch Set 2 : #Patch Set 3 : #
Total comments: 18
Patch Set 4 : CR feedback #Patch Set 5 : Use testing API in ash/test #Patch Set 6 : #Patch Set 7 : #
Total comments: 11
Patch Set 8 : #
Total comments: 3
Patch Set 9 : #
Depends on Patchset: Messages
Total messages: 23 (4 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org
On 2015/07/08 18:03:38, jdufault wrote: > mailto:jdufault@chromium.org changed reviewers: > + mailto:achuith@chromium.org Achuith, can you take a look? Thanks!
https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:18: remove newline https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:32: extensions::ProcessManager* pm = extensions::ProcessManager::Get(profile); You could use auto here https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:33: auto host = Don't think this is appropriate since the type is not clear. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:46: ash::Shell::GetInstance()->GetPrimarySystemTray()->ShowDefaultView( I think it's worthwhile to create a temporary for the tray here: <tray_type> tray = ash::Shell::GetInstance()->GetPrimarySystemTray(); tray->ShowDefaultView(..); return tray->GetTrayCastForTesting(); https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:47: ash::BubbleCreationType::BUBBLE_CREATE_NEW); What's going on here? https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:58: remove newline? https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:70: // recongnizes the cast extension. sp recognizes https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:73: auto cast_config_delegate = ash::Shell::GetInstance() Type is not clear https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:94: auto extension = LoadCastTestExtension(); Type is not clear https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:106: auto extension = LoadCastTestExtension(); Type is not clear. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:107: ExecuteJavaScript(extension, "addReceiver('id', 'name', 'title', 1)"); test_id? https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:110: ExecutePendingJavaScript(extension); I don't understand the rationale for this sequence of calls. Do you have to call them in this order? https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:121: auto extension = LoadCastTestExtension(); Type is not clear https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:134: FAIL(); // Unknown return type for wasStopMirroringCalled(). Why not EXPECT_TRUE(wasStopMirroringCalled->GetAsBoolean(&result)); https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:150: ((ash::SystemTrayItem*)tray) Please use static_cast here https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:153: tray->StartCastForTest("id"); test_id https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:160: if (launchDesktopMirroringReceiverId->GetAsString(&result) == false) { EXPECT_TRUE https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:169: auto cast_config_delegate = ash::Shell::GetInstance() Type is not clear https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:174: auto url = browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); GURL https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:175: EXPECT_TRUE(base::StringPiece(url.GetContent()).ends_with("options.html")); Hmm, this seems a bit fragile since the cast extension could rename the file. I don't have any ideas however.
https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:18: On 2015/07/08 22:04:55, achuithb wrote: > remove newline Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:32: extensions::ProcessManager* pm = extensions::ProcessManager::Get(profile); On 2015/07/08 22:04:55, achuithb wrote: > You could use auto here Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:33: auto host = On 2015/07/08 22:04:55, achuithb wrote: > Don't think this is appropriate since the type is not clear. Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:46: ash::Shell::GetInstance()->GetPrimarySystemTray()->ShowDefaultView( On 2015/07/08 22:04:55, achuithb wrote: > I think it's worthwhile to create a temporary for the tray here: > > <tray_type> tray = ash::Shell::GetInstance()->GetPrimarySystemTray(); > tray->ShowDefaultView(..); > return tray->GetTrayCastForTesting(); Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:47: ash::BubbleCreationType::BUBBLE_CREATE_NEW); On 2015/07/08 22:04:55, achuithb wrote: > What's going on here? I've added a comment. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:58: On 2015/07/08 22:04:55, achuithb wrote: > remove newline? Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:70: // recongnizes the cast extension. On 2015/07/08 22:04:55, achuithb wrote: > sp recognizes Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:73: auto cast_config_delegate = ash::Shell::GetInstance() On 2015/07/08 22:04:55, achuithb wrote: > Type is not clear Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:94: auto extension = LoadCastTestExtension(); On 2015/07/08 22:04:55, achuithb wrote: > Type is not clear Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:106: auto extension = LoadCastTestExtension(); On 2015/07/08 22:04:55, achuithb wrote: > Type is not clear. Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:107: ExecuteJavaScript(extension, "addReceiver('id', 'name', 'title', 1)"); On 2015/07/08 22:04:55, achuithb wrote: > test_id? Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:110: ExecutePendingJavaScript(extension); On 2015/07/08 22:04:55, achuithb wrote: > I don't understand the rationale for this sequence of calls. Do you have to call > them in this order? I've modified GetTrayCast so it takes an extension and makes this ExecutePendingJavaScript call itself. Essentially, when we create the tray it runs some JS queries to see if it should display itself. We have the ExecutePendingJS call to make sure those queries are complete. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:121: auto extension = LoadCastTestExtension(); On 2015/07/08 22:04:55, achuithb wrote: > Type is not clear Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:134: FAIL(); // Unknown return type for wasStopMirroringCalled(). On 2015/07/08 22:04:55, achuithb wrote: > Why not > EXPECT_TRUE(wasStopMirroringCalled->GetAsBoolean(&result)); Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:150: ((ash::SystemTrayItem*)tray) On 2015/07/08 22:04:55, achuithb wrote: > Please use static_cast here Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:153: tray->StartCastForTest("id"); On 2015/07/08 22:04:55, achuithb wrote: > test_id Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:160: if (launchDesktopMirroringReceiverId->GetAsString(&result) == false) { On 2015/07/08 22:04:55, achuithb wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:169: auto cast_config_delegate = ash::Shell::GetInstance() On 2015/07/08 22:04:54, achuithb wrote: > Type is not clear Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:174: auto url = browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); On 2015/07/08 22:04:55, achuithb wrote: > GURL Done. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:175: EXPECT_TRUE(base::StringPiece(url.GetContent()).ends_with("options.html")); On 2015/07/08 22:04:55, achuithb wrote: > Hmm, this seems a bit fragile since the cast extension could rename the file. I > don't have any ideas however. I'm not sure it's even a good test, because the mock extension doesn't have the options page. All it does is verify that a page ending in "options.html" was launched; it does not test if that options page is actually the extensions options page (since it could just be a bad URL).
Could we also verify multiple receivers? I don't know what a good test would be. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:110: ExecutePendingJavaScript(extension); On 2015/07/09 23:12:57, jdufault wrote: > On 2015/07/08 22:04:55, achuithb wrote: > > I don't understand the rationale for this sequence of calls. Do you have to > call > > them in this order? > > I've modified GetTrayCast so it takes an extension and makes this > ExecutePendingJavaScript call itself. Essentially, when we create the tray it > runs some JS queries to see if it should display itself. We have the > ExecutePendingJS call to make sure those queries are complete. Yup, I like that better https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:175: EXPECT_TRUE(base::StringPiece(url.GetContent()).ends_with("options.html")); On 2015/07/09 23:12:57, jdufault wrote: > On 2015/07/08 22:04:55, achuithb wrote: > > Hmm, this seems a bit fragile since the cast extension could rename the file. > I > > don't have any ideas however. > > I'm not sure it's even a good test, because the mock extension doesn't have the > options page. All it does is verify that a page ending in "options.html" was > launched; it does not test if that options page is actually the extensions > options page (since it could just be a bad URL). So the mock extension is not launching the real options.html page?
I've added the multiple receiver test, but in doing so I refactored some other common bits out. PTAL. https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/1/chrome/browser/ui/ash/syste... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:175: EXPECT_TRUE(base::StringPiece(url.GetContent()).ends_with("options.html")); On 2015/07/10 21:32:06, achuithb wrote: > On 2015/07/09 23:12:57, jdufault wrote: > > On 2015/07/08 22:04:55, achuithb wrote: > > > Hmm, this seems a bit fragile since the cast extension could rename the > file. > > I > > > don't have any ideas however. > > > > I'm not sure it's even a good test, because the mock extension doesn't have > the > > options page. All it does is verify that a page ending in "options.html" was > > launched; it does not test if that options page is actually the extensions > > options page (since it could just be a bad URL). > > So the mock extension is not launching the real options.html page? It's not the extension which is launching the options page - the cast config delegate is just opening up a resource called "options.html" in the context of the extension. I looked into changing CastConfigDelegateChromeOS::LaunchCastOptions() to go through the extension, but it would involve exporting another symbol in the extension (which will then take lots of time to propagate out; besides that, I cannot find a common method to launch the options page, so it'll likely have to be added). I think the only time the current LaunchCastOptions() impl will break is if the options.html page gets renamed, which is possible but unlikely.
https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:26: if (!profile) { Can this actually happen? If not, drop the check https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:47: const std::string& id) { receiver_id https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:49: ash::TrayCastTestMethods* test_tray = tray; Is this necessary for compilation? https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:61: scoped_ptr<base::Value> getLaunchDesktopMirroringId = variable name should have underscores, not camel case https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:108: class SystemTrayTrayCastChromeOSTest : public ExtensionBrowserTest { This cannot be in the anonymous namespace, is that right? https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:169: ExecuteJavaScript(extension, "addReceiver('test_id_1', 'name')"); Use different names for variety https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:223: GURL url = browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); const
https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:26: if (!profile) { On 2015/07/14 18:22:20, achuithb wrote: > Can this actually happen? If not, drop the check Done. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:47: const std::string& id) { On 2015/07/14 18:22:19, achuithb wrote: > receiver_id Done. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:49: ash::TrayCastTestMethods* test_tray = tray; On 2015/07/14 18:22:20, achuithb wrote: > Is this necessary for compilation? Yes, since the methods are private w.r.t. ash::TrayCast. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:61: scoped_ptr<base::Value> getLaunchDesktopMirroringId = On 2015/07/14 18:22:20, achuithb wrote: > variable name should have underscores, not camel case Done. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:108: class SystemTrayTrayCastChromeOSTest : public ExtensionBrowserTest { On 2015/07/14 18:22:19, achuithb wrote: > This cannot be in the anonymous namespace, is that right? Oh, I just was following the pattern of another browser test. I've moved the entire file to an anonymous namespace. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:169: ExecuteJavaScript(extension, "addReceiver('test_id_1', 'name')"); On 2015/07/14 18:22:20, achuithb wrote: > Use different names for variety I want to verify that the having the same name doesn't break anything. I've changed two of them to name1\name2. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:223: GURL url = browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); On 2015/07/14 18:22:20, achuithb wrote: > const Done.
https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:49: ash::TrayCastTestMethods* test_tray = tray; On 2015/07/15 17:35:28, jdufault wrote: > On 2015/07/14 18:22:20, achuithb wrote: > > Is this necessary for compilation? > > Yes, since the methods are private w.r.t. ash::TrayCast. Acknowledged. https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:108: class SystemTrayTrayCastChromeOSTest : public ExtensionBrowserTest { On 2015/07/15 17:35:28, jdufault wrote: > On 2015/07/14 18:22:19, achuithb wrote: > > This cannot be in the anonymous namespace, is that right? > > Oh, I just was following the pattern of another browser test. I've moved the > entire file to an anonymous namespace. I think you want the IN_PROC_BROWSER_TEST_F to be in non-anonymous namespace because you can't use gtest_filter otherwise. But the base class can be in the anonymous namespace https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:169: ExecuteJavaScript(extension, "addReceiver('test_id_1', 'name')"); On 2015/07/15 17:35:27, jdufault wrote: > On 2015/07/14 18:22:20, achuithb wrote: > > Use different names for variety > > I want to verify that the having the same name doesn't break anything. I've > changed two of them to name1\name2. Acknowledged.
https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/40001/chrome/browser/ui/ash/s... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:108: class SystemTrayTrayCastChromeOSTest : public ExtensionBrowserTest { On 2015/07/15 18:59:32, achuithb wrote: > On 2015/07/15 17:35:28, jdufault wrote: > > On 2015/07/14 18:22:19, achuithb wrote: > > > This cannot be in the anonymous namespace, is that right? > > > > Oh, I just was following the pattern of another browser test. I've moved the > > entire file to an anonymous namespace. > > I think you want the IN_PROC_BROWSER_TEST_F to be in non-anonymous namespace > because you can't use gtest_filter otherwise. > > But the base class can be in the anonymous namespace I've been testing with --gtest_filter=SystemTrayTrayCastChromeOSTest.* and it still works in the anonymous namespace. Or will it break outside of my machine?
jdufault@chromium.org changed reviewers: + oshima@chromium.org
Oshima, would you mind taking a look? Thanks!
https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:34: ExecuteJavaScript(extension, ""); std::string() https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:43: ash::SystemTrayItem* system_tray_item = tray; is there particular reason why you want to have this separate variable? https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:60: EXPECT_EQ(receiver_id, result); EXPECT in a utility function makes it difficult to understand in which step it's failing. Looks like the 1st EXPECT_TRUE is actually an assert, and the 2nd can be returned as a boolean and the caller can test? https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:78: EXPECT_TRUE(result); ditto https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:221: } // namespace we usually put the test bodies outside of anonymous for couple of reasons: 1) tests names are supposed to be unique (so that gtest_filter will not conflict) 2) you can't make them friend.
https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:34: ExecuteJavaScript(extension, ""); On 2015/07/20 23:07:04, oshima wrote: > std::string() Done. https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:43: ash::SystemTrayItem* system_tray_item = tray; On 2015/07/20 23:07:03, oshima wrote: > is there particular reason why you want to have this separate variable? Yes, the API has private visibility in ash::TrayCast. https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:60: EXPECT_EQ(receiver_id, result); On 2015/07/20 23:07:04, oshima wrote: > EXPECT in a utility function makes it difficult to understand in which step it's > failing. Looks like the 1st EXPECT_TRUE is actually an assert, and the 2nd can > be returned as a boolean and the caller can test? Done. https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:78: EXPECT_TRUE(result); On 2015/07/20 23:07:03, oshima wrote: > ditto Done. https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:221: } // namespace On 2015/07/20 23:07:04, oshima wrote: > we usually put the test bodies outside of anonymous for couple of reasons: > > 1) tests names are supposed to be unique (so that gtest_filter will not > conflict) > 2) you can't make them friend. Done.
can you remove core files? https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:43: ash::SystemTrayItem* system_tray_item = tray; On 2015/07/21 19:52:44, jdufault wrote: > On 2015/07/20 23:07:03, oshima wrote: > > is there particular reason why you want to have this separate variable? > > Yes, the API has private visibility in ash::TrayCast. Acknowledged. https://codereview.chromium.org/1231593002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:27: Profile* profile = ProfileManager::GetActiveUserProfile(); GetactiveUserProfile is deprecated. Can you use multi_user_util::GetProfileFromUserID( multi_user_util::GetCurrentUserId()); instead?
Oops, I didn't mean to add the core files. https://codereview.chromium.org/1231593002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:27: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/07/21 21:21:04, oshima wrote: > GetactiveUserProfile is deprecated. Can you use > > multi_user_util::GetProfileFromUserID( > multi_user_util::GetCurrentUserId()); > > instead? Done.
https://codereview.chromium.org/1231593002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc (right): https://codereview.chromium.org/1231593002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/system_tray_tray_cast_browsertest_chromeos.cc:27: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2015/07/21 23:50:25, jdufault wrote: > On 2015/07/21 21:21:04, oshima wrote: > > GetactiveUserProfile is deprecated. Can you use > > > > multi_user_util::GetProfileFromUserID( > > multi_user_util::GetCurrentUserId()); > > > > instead? > > Done. Oops, sorry - it doesn't work here and just returns nullptr.
lgtm
lgtm
The CQ bit was checked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1231593002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1231593002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
