Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1198313003:
Fix the browser match rules. (Closed)
DescriptionWhen we try to find a browser window based on some matching rules, we need to return
a browser window that are shown on the given |profile|'s desktop to prevent returning
a browser window on other users' desktop.
Also clean up and refactor codes related to TestBrowserWindowAura class. There were
multiple files defining TestBrowserWindowAura that were almost identical. Extracted this class to
test_browser_window.h .cc files and added helper function CreateBrowserWithAuraTestWindowForParams()
to handle its lifetime.
BUG=500027
Committed: https://crrev.com/69630960fee17f0fa81b47b81625da31055daa7a
Cr-Commit-Position: refs/heads/master@{#339809}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 26
Patch Set 3 : Address msw@'s comments. #
Total comments: 10
Patch Set 4 : Address msw@'s comments. #Patch Set 5 : Wrap TestBrowserWindowAura inside of (USE_AURA) to fix test failure. #Patch Set 6 : #
Total comments: 28
Patch Set 7 : Address msw@'s comments. #
Total comments: 8
Patch Set 8 : Address msw@'s comments. #
Total comments: 1
Patch Set 9 : Revert back to use Browesr raw pointer as the return type for CreateBrowserWithTestWindowForParams #
Total comments: 3
Patch Set 10 : Use scoped_ptr<Browser> instead. #
Total comments: 16
Patch Set 11 : Address msw@'s comments. #
Total comments: 2
Patch Set 12 : Move TestBrowserWindowAura into its own .h .cc files #
Total comments: 2
Patch Set 13 : Address sky@'s comment. #Messages
Total messages: 45 (13 generated)
xdai@chromium.org changed reviewers: + msw@chromium.org
msw@, could you help to review the CL please? Thanks!
Do we have any browser finder tests? A test for this would be good. Please add/CC a reviewer with more profile knowledge. https://codereview.chromium.org/1198313003/diff/1/chrome/browser/ui/browser_f... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1198313003/diff/1/chrome/browser/ui/browser_f... chrome/browser/ui/browser_finder.cc:64: shown_profile == profile->GetOriginalProfile(); Should you use shown_profile->GetOriginalProfile()? https://codereview.chromium.org/1198313003/diff/1/chrome/browser/ui/browser_f... chrome/browser/ui/browser_finder.cc:64: shown_profile == profile->GetOriginalProfile(); Should this change be limited to when kMatchOriginalProfile is specified? (your cl description seems to indicate that's the case)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
xdai@chromium.org changed reviewers: + sky@chromium.org
Thanks for the review! Please take anther look. +sky to decide if it's a proper behavior for BrowserMatches() function to exclude browser windows that are shown on other user's desktop. https://codereview.chromium.org/1198313003/diff/1/chrome/browser/ui/browser_f... File chrome/browser/ui/browser_finder.cc (right): https://codereview.chromium.org/1198313003/diff/1/chrome/browser/ui/browser_f... chrome/browser/ui/browser_finder.cc:64: shown_profile == profile->GetOriginalProfile(); On 2015/06/23 17:08:51, msw wrote: > Should you use shown_profile->GetOriginalProfile()? Sorry my fault. Should compare with original profile.
The test file seems far more complex than necessary; can you try to simplify it by using more existing test helpers? https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:7: #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h" This doesn't appear to be used/needed. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:12: #include "chrome/browser/ui/browser_list.h" This doesn't appear to be used/needed. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:25: class TestBrowserWindowAura : public TestBrowserWindow { Should this file include chrome/test/base/test_browser_window.h? https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:56: BrowserFinderChromeOSTest() : multi_user_window_manager_(NULL) {} nit: nullptr https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:58: TestingProfile* CreateMultiUserProfile(const std::string& user_email) { Why not inline this in the CreateProfile override and keep that protected? https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:60: profile_manager()->CreateTestingProfile(user_email); nit: use profile_manager_ and remove the accessor. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:66: scoped_ptr<TestBrowserWindowAura> CreateBrowserWindowWithProfile( This and TestBrowserWindowAura seem very odd to me... Please try to reuse more existing test utility code to simplify this file. Try using BrowserWithTestWindowTest::CreateBrowser (with profile()->GetOriginalProfile()/GetOffTheRecordProfile() as needed), use [Test]BrowserWindow::GetNativeWindow() to avoid the need for TestBrowserWindowAura, etc. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:108: chromeos::WallpaperManager::Initialize(); q: why do we need this? https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:138: typedef std::map<Profile*, std::string> ProfileToNameMap; Why is this needed? Can't you just use GetProfileUserName() and keep an std::set of profiles (or really just keep one extra scoped_ptr or raw pointer of the second profile that you need, besides the BrowserWithTestWindowTest's member)? https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:142: // Test the matching rules for incognito browser. nit: this comment doesn't add much value, remove it https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:165: TEST_F(BrowserFinderChromeOSTest, MultiProfileBrowserMatchTest) { nit: name this test something like "FindBrowserOwnedByAnotherProfile" https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:183: // be avaiable for the current profile. nit: available https://codereview.chromium.org/1198313003/diff/60001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1198313003/diff/60001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:1351: 'browser/ui/browser_finder_chromeos_unittest.cc', nit: zturner's comment likely applies to immersive_mode_controller_ash_unittest.cc; put this new entry above his comment to avoid misdirecting its intent (unless it's also applicable here?).
Patchset #3 (id:80001) has been deleted
msw@, thanks for the review, please take another look! https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:7: #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h" On 2015/06/30 18:42:33, msw wrote: > This doesn't appear to be used/needed. Removed. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:12: #include "chrome/browser/ui/browser_list.h" On 2015/06/30 18:42:33, msw wrote: > This doesn't appear to be used/needed. Removed. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:25: class TestBrowserWindowAura : public TestBrowserWindow { On 2015/06/30 18:42:33, msw wrote: > Should this file include chrome/test/base/test_browser_window.h? It has been included in "chrome/test/base/browser_with_test_window_test.h" file. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:56: BrowserFinderChromeOSTest() : multi_user_window_manager_(NULL) {} On 2015/06/30 18:42:34, msw wrote: > nit: nullptr Done. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:58: TestingProfile* CreateMultiUserProfile(const std::string& user_email) { On 2015/06/30 18:42:33, msw wrote: > Why not inline this in the CreateProfile override and keep that protected? CreateProfile() is override from BrowserWithTestWindowTest and will be called in BrowserWithTestWindowTest::SetUp() to create a default profile. I think it's necessary to have a function that could create another profile. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:60: profile_manager()->CreateTestingProfile(user_email); On 2015/06/30 18:42:34, msw wrote: > nit: use profile_manager_ and remove the accessor. Done. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:66: scoped_ptr<TestBrowserWindowAura> CreateBrowserWindowWithProfile( On 2015/06/30 18:42:33, msw wrote: > This and TestBrowserWindowAura seem very odd to me... Please try to reuse more > existing test utility code to simplify this file. Try using > BrowserWithTestWindowTest::CreateBrowser (with > profile()->GetOriginalProfile()/GetOffTheRecordProfile() as needed), use > [Test]BrowserWindow::GetNativeWindow() to avoid the need for > TestBrowserWindowAura, etc. In BrowserWithTestWindowTest, the |window_| is usually a TestBrowserWindow, which doesn't have a native window, and that's why I use TestBrowserWindowAura class to associated a native window with it. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:108: chromeos::WallpaperManager::Initialize(); On 2015/06/30 18:42:33, msw wrote: > q: why do we need this? This will be used in CreateMultiUserProfile() function, see line 62 in this patch. Add another user into the session will cause user transition animation, which requires wallpaper transition as well. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:138: typedef std::map<Profile*, std::string> ProfileToNameMap; On 2015/06/30 18:42:33, msw wrote: > Why is this needed? Can't you just use GetProfileUserName() and keep an std::set > of profiles (or really just keep one extra scoped_ptr or raw pointer of the > second profile that you need, besides the BrowserWithTestWindowTest's member)? Removed this and maintained a raw pointer of the second profile instead. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:142: // Test the matching rules for incognito browser. On 2015/06/30 18:42:33, msw wrote: > nit: this comment doesn't add much value, remove it Done. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:165: TEST_F(BrowserFinderChromeOSTest, MultiProfileBrowserMatchTest) { On 2015/06/30 18:42:33, msw wrote: > nit: name this test something like "FindBrowserOwnedByAnotherProfile" Done. https://codereview.chromium.org/1198313003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_finder_chromeos_unittest.cc:183: // be avaiable for the current profile. On 2015/06/30 18:42:33, msw wrote: > nit: available Done. https://codereview.chromium.org/1198313003/diff/60001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1198313003/diff/60001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:1351: 'browser/ui/browser_finder_chromeos_unittest.cc', On 2015/06/30 18:42:34, msw wrote: > nit: zturner's comment likely applies to > immersive_mode_controller_ash_unittest.cc; put this new entry above his comment > to avoid misdirecting its intent (unless it's also applicable here?). Sorry didn't notice that. Done.
https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:23: class TestBrowserWindowAura : public TestBrowserWindow { I now see that three other files define TestBrowserWindowAura: chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc chrome/browser/ui/ash/window_positioner_unittest.cc chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc This duplicates the last one, can they (at the least) be merged? (ditto for CreateBrowserWindowWithProfile), which is almost identical. Bonus points if this can be merged into BrowserWithTestWindowTest somehow. Otherwise, I wonder if this should be an InProcessBrowserTest, or something else that supports actual native windows. Maybe Scott has an opinion here. https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:67: aura::Window* window = new aura::Window(NULL); nit: nullptr https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:107: // Create a second profile. TearDown should probably explicitly destroy this profile. https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:117: // Override BrowserWithTestWindowTest. nit: SetUp and TearDown also override BrowserWithTestWindowTest (perhaps move this comment above SetUp, or remove it) https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:123: profile_manager_->DeleteAllTestingProfiles(); Deleting all testing profiles doesn't seem like the intent of this function. It wouldn't break anything for now (afaict), but it might cause problems later. Why not just do: profile_manager_->DeleteTestingProfile(test_profile->GetProfileUserName())
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
msw@, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:23: class TestBrowserWindowAura : public TestBrowserWindow { On 2015/07/01 19:55:16, msw wrote: > I now see that three other files define TestBrowserWindowAura: > chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc > chrome/browser/ui/ash/window_positioner_unittest.cc > chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc > This duplicates the last one, can they (at the least) be merged? (ditto for > CreateBrowserWindowWithProfile), which is almost identical. Bonus points if this > can be merged into BrowserWithTestWindowTest somehow. Otherwise, I wonder if > this should be an InProcessBrowserTest, or something else that supports actual > native windows. Maybe Scott has an opinion here. Extracted the class TestBrowserWindowAura into test_browser_window.h .cc files with some minor adjustments. https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:67: aura::Window* window = new aura::Window(NULL); On 2015/07/01 19:55:16, msw wrote: > nit: nullptr Done. https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:107: // Create a second profile. On 2015/07/01 19:55:16, msw wrote: > TearDown should probably explicitly destroy this profile. Done. https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:117: // Override BrowserWithTestWindowTest. On 2015/07/01 19:55:16, msw wrote: > nit: SetUp and TearDown also override BrowserWithTestWindowTest > (perhaps move this comment above SetUp, or remove it) Removed it. https://codereview.chromium.org/1198313003/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:123: profile_manager_->DeleteAllTestingProfiles(); On 2015/07/01 19:55:16, msw wrote: > Deleting all testing profiles doesn't seem like the intent of this function. It > wouldn't break anything for now (afaict), but it might cause problems later. Why > not just do: > profile_manager_->DeleteTestingProfile(test_profile->GetProfileUserName()) Done.
This is great cleanup; thank you! Mostly minor comments. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:47: WindowPositioner* window_positioner_; nit: make this a scoped_ptr too, please https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:66: aura::Window* window = CreateTestWindowInShellWithId(0); These should still be scoped_ptrs. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:41: } else { nit: no else after return https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/wind... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/wind... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:459: ->ActivateWindow(browser_window->GetNativeWindow()); nit: i wonder if these activations are actually necessary... maybe add an activate (or show and activate) helper in this file? https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... File chrome/test/base/test_browser_window.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:274: // Create a window. nit: remove comment. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:15: #include "ui/aura/window.h" nit: limit to #if defined(USE_AURA) (or add test_browser_window_aura files) https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:203: #if defined(USE_AURA) nit: order this before CreateBrowserWithTestWindowForParams https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:204: // A browser window proxy which is able to associate an aura native window with nit: // A browser window proxy with an associated Aura native window. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:208: // |native_window| will still be owned by the caller after the constructor nit: // The caller retains ownership of |native_window|. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:229: // Helper that create a test browser window which has a native window. nit: creates https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:230: scoped_ptr<TestBrowserWindowAura> CreateBrowserWithNativeWindowForParams( nit: maybe return a Browser* like CreateBrowserWithTestWindowForParams and call this CreateBrowserWithAuraTestWindowForParams? https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:230: scoped_ptr<TestBrowserWindowAura> CreateBrowserWithNativeWindowForParams( nit: move this into the chrome namespace, and order it just after CreateBrowserWithTestWindowForParams
Patchset #7 (id:260001) has been deleted
msw@, I've addressed the comments, please take another look, thanks for the review! https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:47: WindowPositioner* window_positioner_; On 2015/07/07 18:11:54, msw wrote: > nit: make this a scoped_ptr too, please Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:66: aura::Window* window = CreateTestWindowInShellWithId(0); On 2015/07/07 18:11:54, msw wrote: > These should still be scoped_ptrs. It seems |window| is owned by TestBrowserWindowAura class, so the life time of |window| will be taken care by TestBrowserWindowAura? https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:41: } else { On 2015/07/07 18:11:54, msw wrote: > nit: no else after return Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/wind... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/wind... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:459: ->ActivateWindow(browser_window->GetNativeWindow()); On 2015/07/07 18:11:55, msw wrote: > nit: i wonder if these activations are actually necessary... maybe add an > activate (or show and activate) helper in this file? I tried to delete these activations and it makes no differences. So it might be safe to remove these activations? https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... File chrome/test/base/test_browser_window.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:274: // Create a window. On 2015/07/07 18:11:55, msw wrote: > nit: remove comment. Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:15: #include "ui/aura/window.h" On 2015/07/07 18:11:55, msw wrote: > nit: limit to #if defined(USE_AURA) (or add test_browser_window_aura files) Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:203: #if defined(USE_AURA) On 2015/07/07 18:11:55, msw wrote: > nit: order this before CreateBrowserWithTestWindowForParams Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:204: // A browser window proxy which is able to associate an aura native window with On 2015/07/07 18:11:55, msw wrote: > nit: // A browser window proxy with an associated Aura native window. Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:208: // |native_window| will still be owned by the caller after the constructor On 2015/07/07 18:11:55, msw wrote: > nit: // The caller retains ownership of |native_window|. Sorry I think I made a mistake here. I think |native_window| is owned by the class TestBrowserWindowAura? https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:229: // Helper that create a test browser window which has a native window. On 2015/07/07 18:11:55, msw wrote: > nit: creates Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:230: scoped_ptr<TestBrowserWindowAura> CreateBrowserWithNativeWindowForParams( On 2015/07/07 18:11:55, msw wrote: > nit: move this into the chrome namespace, and order it just after > CreateBrowserWithTestWindowForParams Done. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:230: scoped_ptr<TestBrowserWindowAura> CreateBrowserWithNativeWindowForParams( On 2015/07/07 18:11:55, msw wrote: > nit: maybe return a Browser* like CreateBrowserWithTestWindowForParams and call > this CreateBrowserWithAuraTestWindowForParams? Done.
Jeez, it's too bad this code was such a mess... Thanks for working to make it clearer and more sane. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:66: aura::Window* window = CreateTestWindowInShellWithId(0); On 2015/07/08 00:26:21, xdai1 wrote: > On 2015/07/07 18:11:54, msw wrote: > > These should still be scoped_ptrs. > > It seems |window| is owned by TestBrowserWindowAura class, so the life time of > |window| will be taken care by TestBrowserWindowAura? Hmm, that does seem to be the case. The transfer of ownership would be clearer here if the constructor took a scoped_ptr<aura::Window>, but maybe it's fine as-is for now; wdyt? Also, it'd be nice if the ctor had fewer callers and tests used CreateBrowserWithAuraTestWindowForParams instead. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/wind... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/wind... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:459: ->ActivateWindow(browser_window->GetNativeWindow()); On 2015/07/08 00:26:21, xdai1 wrote: > On 2015/07/07 18:11:55, msw wrote: > > nit: i wonder if these activations are actually necessary... maybe add an > > activate (or show and activate) helper in this file? > > I tried to delete these activations and it makes no differences. So it might be > safe to remove these activations? yeah, remove them if they're not needed. https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:208: // |native_window| will still be owned by the caller after the constructor On 2015/07/08 00:26:22, xdai1 wrote: > On 2015/07/07 18:11:55, msw wrote: > > nit: // The caller retains ownership of |native_window|. > > Sorry I think I made a mistake here. I think |native_window| is owned by the > class TestBrowserWindowAura? Ah, that does seem to be the case. https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:92: browser_window_.reset(nullptr); nit: use reset() without a nullptr argument, here and in the two lines below. https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/wind... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/wind... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:491: // Explicitly delete browsers since they're not owned by Hmm, should TestBrowserWindowAura own its browsers, as the old implementations did? Regardless, CreateTestBrowserWindow should return a scoped_ptr<Browser>, so these callers can then manage the Browser lifetimes with local scoped_ptrs. https://codereview.chromium.org/1198313003/diff/280001/chrome/test/base/test_... File chrome/test/base/test_browser_window.cc (right): https://codereview.chromium.org/1198313003/diff/280001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:9: #include "ui/aura/window.h" nit: this isn't needed, given the include in the header, but if you want it here, it should only be used for USE_AURA https://codereview.chromium.org/1198313003/diff/280001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:51: aura::Window* window = new aura::Window(nullptr); nit: make |window| a scoped_ptr and pass into the ctor for clearer ownership.
msw@, I've addressed your comments, please take another look, thanks for your review! In this patch: 1) Modified the ctor of TestBrowserWindowAura to take a scoped_ptr<aura::Window> argument; 2) Introduced a new function CreateBrowserForAuraTestWindowAndParams(scoped_ptr<aura::Window>, const Browser::CreateParams&); 3) Modified the return type of CreateBrowserWithTestWindowForParams and CreateBrowserWithAuraTestWindowForParams from Browser raw pointer to scoped_ptr<Browser>. https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/240001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:66: aura::Window* window = CreateTestWindowInShellWithId(0); On 2015/07/08 01:13:59, msw wrote: > On 2015/07/08 00:26:21, xdai1 wrote: > > On 2015/07/07 18:11:54, msw wrote: > > > These should still be scoped_ptrs. > > > > It seems |window| is owned by TestBrowserWindowAura class, so the life time of > > |window| will be taken care by TestBrowserWindowAura? > > Hmm, that does seem to be the case. The transfer of ownership would be clearer > here if the constructor took a scoped_ptr<aura::Window>, but maybe it's fine > as-is for now; wdyt? Also, it'd be nice if the ctor had fewer callers and tests > used CreateBrowserWithAuraTestWindowForParams instead. Yes, I agree it would be more clearer: modifying the constructor to take a scoped_ptr<aura::Window> and transfer ownership here. But we can't use CreateBrowserWithAuraTestWindowForParams here because in this test the aura window is created by CreateTestWindowInShellWithId. Same reason for WindowSizerAshTest. I think we could create another function accepting a window as an argument like CreateBrowserForAuraTestWindowAndParams(scoped_ptr<aura::Window>, const Browser::CreateParams&) to avoid explicitly calling the ctor, wdyt? https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:92: browser_window_.reset(nullptr); On 2015/07/08 01:14:00, msw wrote: > nit: use reset() without a nullptr argument, here and in the two lines below. Done. https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/wind... File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/wind... chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:491: // Explicitly delete browsers since they're not owned by On 2015/07/08 01:14:00, msw wrote: > Hmm, should TestBrowserWindowAura own its browsers, as the old implementations > did? Regardless, CreateTestBrowserWindow should return a scoped_ptr<Browser>, so > these callers can then manage the Browser lifetimes with local scoped_ptrs. This is a little tricky here. Since we modified CreateBrowserWithAuraTestWindowForParams to return a scoped_ptr<Browser> but not a scoped_ptr<TestBrowserWindowAura>, so the life time of TestBrowserWindowAura will be managed by TestBrowserWindowOwner (see test_browser_window.cc file), which is a BrowserListObserver. TestBrowserWindowOwner will listen to OnBrowserRemoved() event and delete the corresponding TestBrowserWindowAura. So if TestBrowserWindowAura own its browser, it will cause problems. As you suggested, I modified CreateBrowserWithTestWindowForParams, CreateBrowserWithAuraTestWindowForParams to return scoped_ptr<Browser>. Also introduced a new function CreateBrowserForAuraTestWindowAndParams(scoped_ptr<aura::Window>,Browser::CreateParams&) to avoid deleting these browsers explicitly. Do you think it's necessory? https://codereview.chromium.org/1198313003/diff/280001/chrome/test/base/test_... File chrome/test/base/test_browser_window.cc (right): https://codereview.chromium.org/1198313003/diff/280001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:9: #include "ui/aura/window.h" On 2015/07/08 01:14:00, msw wrote: > nit: this isn't needed, given the include in the header, but if you want it > here, it should only be used for USE_AURA Sorry missed this one. Removed it. https://codereview.chromium.org/1198313003/diff/280001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:51: aura::Window* window = new aura::Window(nullptr); On 2015/07/08 01:14:00, msw wrote: > nit: make |window| a scoped_ptr and pass into the ctor for clearer ownership. Done. https://codereview.chromium.org/1198313003/diff/290001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/290001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:225: scoped_ptr<Browser> CreateBrowserWithTestWindowForParams( For consistency, also made CreateBrowserWithTestWindowForParams return a scoped_ptr<Browser>.
Sorry I think I misunderstood your previous comment below. Revert the return type of CreateBrowserWithTestWindowForParams and CreateBrowserWithAuraTestWindowForParams to Browser raw pointer. > > https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/wind... > File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right): > > https://codereview.chromium.org/1198313003/diff/280001/chrome/browser/ui/wind... > chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:491: // Explicitly > delete browsers since they're not owned by > On 2015/07/08 01:14:00, msw wrote: > > Hmm, should TestBrowserWindowAura own its browsers, as the old implementations > > did? Regardless, CreateTestBrowserWindow should return a scoped_ptr<Browser>, > so > > these callers can then manage the Browser lifetimes with local scoped_ptrs. > > This is a little tricky here. Since we modified > CreateBrowserWithAuraTestWindowForParams to return a scoped_ptr<Browser> but not > a scoped_ptr<TestBrowserWindowAura>, so the life time of TestBrowserWindowAura > will be managed by TestBrowserWindowOwner (see test_browser_window.cc file), > which is a BrowserListObserver. TestBrowserWindowOwner will listen to > OnBrowserRemoved() event and delete the corresponding TestBrowserWindowAura. So > if TestBrowserWindowAura own its browser, it will cause problems. > > As you suggested, I modified CreateBrowserWithTestWindowForParams, > CreateBrowserWithAuraTestWindowForParams to return scoped_ptr<Browser>. > > Also introduced a new function > CreateBrowserForAuraTestWindowAndParams(scoped_ptr<aura::Window>,Browser::CreateParams&) > to avoid deleting these browsers explicitly. Do you think it's necessory? >
On 2015/07/08 22:52:47, xdai1 wrote: > Sorry I think I misunderstood your previous comment below. Revert the return > type of CreateBrowserWithTestWindowForParams and > CreateBrowserWithAuraTestWindowForParams to Browser raw pointer. Please do have them return scoped_ptr! https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... File chrome/test/base/test_browser_window.cc (right): https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:56: TestBrowserWindowAura* browser_window = nit: if we keep this function, make it just call CreateBrowserForAuraTestWindowAndParams here (instead of duplicating this code). https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... chrome/test/base/test_browser_window.cc:64: const Browser::CreateParams& params) { Should these functions take a params pointer, like CreateBrowserWithTestWindowForParams? https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:235: Browser* CreateBrowserForAuraTestWindowAndParams( Could we just have this single function, and callers could just pass in a null window if they want one created for them?
msw@, I've addressed your comments, please take another look, thanks! On 2015/07/08 23:58:00, msw wrote: > On 2015/07/08 22:52:47, xdai1 wrote: > > Sorry I think I misunderstood your previous comment below. Revert the return > > type of CreateBrowserWithTestWindowForParams and > > CreateBrowserWithAuraTestWindowForParams to Browser raw pointer. > > Please do have them return scoped_ptr! Done. > > https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... > File chrome/test/base/test_browser_window.cc (right): > > https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... > chrome/test/base/test_browser_window.cc:56: TestBrowserWindowAura* > browser_window = > nit: if we keep this function, make it just call > CreateBrowserForAuraTestWindowAndParams here (instead of duplicating this code). Removed this function. > > https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... > chrome/test/base/test_browser_window.cc:64: const Browser::CreateParams& params) > { > Should these functions take a params pointer, like > CreateBrowserWithTestWindowForParams? Done. > > https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... > File chrome/test/base/test_browser_window.h (right): > > https://codereview.chromium.org/1198313003/diff/310001/chrome/test/base/test_... > chrome/test/base/test_browser_window.h:235: Browser* > CreateBrowserForAuraTestWindowAndParams( > Could we just have this single function, and callers could just pass in a null > window if they want one created for them? Done.
lgtm with mostly minor nits remaining. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/power/p... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/power/p... chrome/browser/power/process_power_collector_unittest.cc:126: scoped_ptr<Browser> browser2 = nit: All these callsites in the CL can still use the copy ctor... https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:79: popup.Pass(), &popup_params); nit: is this formatted correctly? https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:83: browser_->window()->GetNativeWindow()->Hide(); nit: use window() and popup() here and below. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:34: scoped_ptr<Browser> CreateBrowserWithProfile(Profile* profile, nit: this is only called twice, once with |is_incognito| false, and once true... maybe just inline the necessary parts of this function at the callsites? https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:66: // Create a second profile. nit: this comment isn't helpful. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller_unittest.mm (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller_unittest.mm:103: Browser::CreateParams native_params(profile(), chrome::GetActiveDesktop()); nit: s/native_params/params/ for a one-liner below. https://codereview.chromium.org/1198313003/diff/330001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:229: // Helper that creates a browser with a native aura |window|. If |window| is nit: capitalize Aura https://codereview.chromium.org/1198313003/diff/330001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:230: // nullptr, it will create an aura window to associate with the browser. It also nit: capitalize Aura
Please update the CL description too.
msw@, I've addressed your comments. Thanks for your review! I'll wait sky@'s response to decide if it's a proper behavior for BrowserMatches() in chrome/browser/ui/browser_finder.cc file. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/power/p... File chrome/browser/power/process_power_collector_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/power/p... chrome/browser/power/process_power_collector_unittest.cc:126: scoped_ptr<Browser> browser2 = On 2015/07/09 22:37:37, msw wrote: > nit: All these callsites in the CL can still use the copy ctor... Reverted it back.... https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/window_positioner_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:79: popup.Pass(), &popup_params); On 2015/07/09 22:37:37, msw wrote: > nit: is this formatted correctly? Yes, it is. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/ash/... chrome/browser/ui/ash/window_positioner_unittest.cc:83: browser_->window()->GetNativeWindow()->Hide(); On 2015/07/09 22:37:37, msw wrote: > nit: use window() and popup() here and below. Done. Renamed local variable |window| and |popup| to avoid conflict. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/brow... File chrome/browser/ui/browser_finder_chromeos_unittest.cc (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:34: scoped_ptr<Browser> CreateBrowserWithProfile(Profile* profile, On 2015/07/09 22:37:37, msw wrote: > nit: this is only called twice, once with |is_incognito| false, and once true... > maybe just inline the necessary parts of this function at the callsites? Done. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/brow... chrome/browser/ui/browser_finder_chromeos_unittest.cc:66: // Create a second profile. On 2015/07/09 22:37:37, msw wrote: > nit: this comment isn't helpful. Removed it. https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller_unittest.mm (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_editor_base_controller_unittest.mm:103: Browser::CreateParams native_params(profile(), chrome::GetActiveDesktop()); On 2015/07/09 22:37:37, msw wrote: > nit: s/native_params/params/ for a one-liner below. Done. https://codereview.chromium.org/1198313003/diff/330001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/330001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:229: // Helper that creates a browser with a native aura |window|. If |window| is On 2015/07/09 22:37:37, msw wrote: > nit: capitalize Aura Done. https://codereview.chromium.org/1198313003/diff/330001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:230: // nullptr, it will create an aura window to associate with the browser. It also On 2015/07/09 22:37:37, msw wrote: > nit: capitalize Aura Done.
sky@, could you help to review chrome/browser/ui/browser_finder.cc file to decide if the browser matching rules are reasonable? Thanks!
I don't understand the subtlety with GetUserPresentingWindow to adequately review this. Can you get whoever added that code to review this?
xdai@chromium.org changed reviewers: + skuhne@chromium.org
On 2015/07/20 18:27:38, sky wrote: > I don't understand the subtlety with GetUserPresentingWindow to adequately > review this. Can you get whoever added that code to review this? Thanks. +skuhne@, could you help to review chrome/browser/ui/browser_finder.cc file to decide if the browser matching rules are reasonable?
lgtm
On 2015/07/20 19:53:04, Mr4D wrote: > lgtm skuhne@, thanks for the review! sky@, could you help to do an owner review for chrome/test/base/test_browser_window.h .cc files? Thanks.
https://codereview.chromium.org/1198313003/diff/350001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/350001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:201: class TestBrowserWindowAura : public TestBrowserWindow { Can this be moved to the test?
https://codereview.chromium.org/1198313003/diff/350001/chrome/test/base/test_... File chrome/test/base/test_browser_window.h (right): https://codereview.chromium.org/1198313003/diff/350001/chrome/test/base/test_... chrome/test/base/test_browser_window.h:201: class TestBrowserWindowAura : public TestBrowserWindow { On 2015/07/20 23:09:08, sky wrote: > Can this be moved to the test? I asked for this code to be consolidated, refined, and added here. Otherwise, we'll have 4 near-duplicate implementations of this code: chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc chrome/browser/ui/ash/window_positioner_unittest.cc chrome/browser/ui/browser_finder_chromeos_unittest.cc chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc
I hadn't realized it was used by more than one test, so, that sounds reasonable. Can we at least move to it's own header, eg test_browser_window_aura?
On 2015/07/20 23:51:42, sky wrote: > I hadn't realized it was used by more than one test, so, that sounds reasonable. > Can we at least move to it's own header, eg test_browser_window_aura? Sure. I can do that. On a second thought, I think TestBrowserWindowAura class might be able to move to the anonymous namespace in chrome/test/base/test_browser_window.cc file, because it is only used in CreateBrowserWithAuraTestWindowForParams() function. What do you think?
I prefer moving both into their own .h/.cc, eg CreateBrowserWithAuraTestWindowForParams goes in test_browser_window_aura.h and TestBrowserWindowAura should go into test_browser_window_aura.cc. -Scott On Mon, Jul 20, 2015 at 4:56 PM, <xdai@chromium.org> wrote: > On 2015/07/20 23:51:42, sky wrote: >> >> I hadn't realized it was used by more than one test, so, that sounds > > reasonable. >> >> Can we at least move to it's own header, eg test_browser_window_aura? > > > Sure. I can do that. On a second thought, I think TestBrowserWindowAura > class > might be able to move to the anonymous namespace in > chrome/test/base/test_browser_window.cc file, because it is only used in > CreateBrowserWithAuraTestWindowForParams() function. What do you think? > > https://codereview.chromium.org/1198313003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've addressed the comment. Please take another look, thanks for your review. On 2015/07/21 14:36:35, sky wrote: > I prefer moving both into their own .h/.cc, eg > CreateBrowserWithAuraTestWindowForParams goes in > test_browser_window_aura.h and TestBrowserWindowAura should go into > test_browser_window_aura.cc. > > -Scott >
LGTM https://codereview.chromium.org/1198313003/diff/370001/chrome/test/base/test_... File chrome/test/base/test_browser_window_aura.cc (right): https://codereview.chromium.org/1198313003/diff/370001/chrome/test/base/test_... chrome/test/base/test_browser_window_aura.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1198313003/diff/370001/chrome/test/base/test_... File chrome/test/base/test_browser_window_aura.h (right): https://codereview.chromium.org/1198313003/diff/370001/chrome/test/base/test_... chrome/test/base/test_browser_window_aura.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c)
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, skuhne@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1198313003/#ps390001 (title: "Address sky@'s comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1198313003/390001
Message was sent while issue was closed.
Committed patchset #13 (id:390001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/69630960fee17f0fa81b47b81625da31055daa7a Cr-Commit-Position: refs/heads/master@{#339809} |
