Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7706027:
Addition testing for panels drag and drop. (Closed)
|
Created:
9 years, 4 months ago by prasadt Modified:
9 years, 4 months ago CC:
chromium-reviews, jianli, Dmitry Titov, dcheng, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAddition testing for panels drag and drop.
- Added testing for drag and drop involving multiple panels.
- Refactored the code to make it clear what's being tested.
- Refactored to simplify the test code so we don't worry about bugs in
test code. Comes with some loss of compactness but is the right
trade-off for test code.
TEST=none
BUG=none
modified: chrome/browser/ui/panels/panel_browsertest.cc
modified: chrome/browser/ui/panels/panel_manager.cc
modified: chrome/browser/ui/panels/panel_manager.h
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98314
Patch Set 1 #
Total comments: 31
Patch Set 2 : Code review feedback. #Patch Set 3 : Code review feedback. #Patch Set 4 : Removed comment markers and used scoping and indentation for delimiting. #
Total comments: 4
Patch Set 5 : Fix windows test failures. #
Total comments: 53
Patch Set 6 : Code review feedback. Mostly cosmetic. #
Total comments: 8
Patch Set 7 : More code review feedback. #
Total comments: 6
Patch Set 8 : Code review feedback. Fix windows failure. #Patch Set 9 : For testing on windows #
Messages
Total messages: 18 (0 generated)
Drive by... http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:174: panels[i]->Close(); Is it safe to modify the vector as you're iterating through it? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:201: enum HandleDrag { Would DragAction be more descriptive? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:216: // These are bounds at the beginning of this test. This would be different I'm still having trouble understanding how initial_bounds would be different from beginning bounds. Maybe an example would help? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:225: if (handle_drag & HANDLE_DRAG_BEGIN) { Oh, it's a bitmask. Could you add a comment to the enum stating that? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:318: // **** Tests with a single panel. Not a big fan of extra **** or ==== in the comments... Why not create multiple tests - OnePanelDrag, TwoPanelsDrag, ThreePanelsDrag? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:419: CreatePanel("PanelTest2", gfx::Rect(0, 0, 110, 110)); PanelTest3 http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_manager.h:87: static int horizontal_spacing(); Need comment explaining what this is.
Some initial comments. More coming later. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:30: using base::PlatformThread; Why is need needed? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:311: int max_panels = 4; You can add "static const" for this variable. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:355: CreatePanel("PanelTest2", gfx::Rect(0, 0, 90, 90)); For now, do not create the panel with size smaller than 100x100. On windows, the minimum window can be created is 100x100. Probably this is the reason you have failed test on win trybot. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:568: RestoreWorkArea(); No need to restore work area. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_manager.cc:11: #include "base/threading/platform_thread.h" Why is this include needed? http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_manager.h:87: static int horizontal_spacing(); Since this is only needed by the test, you should not make it accessible to everyone. Either try to use UNIT_TEST to guard it or move it to private and expose it to FRIEND test.
Thanks for all the feedback. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:30: using base::PlatformThread; On 2011/08/23 17:27:12, jianli wrote: > Why is need needed? Not needed. I was using it to get the Sleep method for some debugging forgot to clean it up. Removed it. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:174: panels[i]->Close(); On 2011/08/23 17:14:16, jennb wrote: > Is it safe to modify the vector as you're iterating through it? We're not modifying the vector, rather the object stored in the vector. The operation we're doing is safe. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:201: enum HandleDrag { On 2011/08/23 17:14:16, jennb wrote: > Would DragAction be more descriptive? Sounds good. Done. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:216: // These are bounds at the beginning of this test. This would be different On 2011/08/23 17:14:16, jennb wrote: > I'm still having trouble understanding how initial_bounds would be different > from beginning bounds. Maybe an example would help? Done. I added an example, hope that helps. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:225: if (handle_drag & HANDLE_DRAG_BEGIN) { On 2011/08/23 17:14:16, jennb wrote: > Oh, it's a bitmask. Could you add a comment to the enum stating that? Done. There is a comment already that says its a set of flags. Added bit mask to the wording. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:311: int max_panels = 4; On 2011/08/23 17:27:12, jianli wrote: > You can add "static const" for this variable. Done. Also changed the value to 3. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:318: // **** Tests with a single panel. On 2011/08/23 17:14:16, jennb wrote: > Not a big fan of extra **** or ==== in the comments... Sure. They make sense here though. > Why not create multiple > tests - OnePanelDrag, TwoPanelsDrag, ThreePanelsDrag? It'll make the tests much slower as we start and shutdown the browser for each test. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:355: CreatePanel("PanelTest2", gfx::Rect(0, 0, 90, 90)); On 2011/08/23 17:27:12, jianli wrote: > For now, do not create the panel with size smaller than 100x100. On windows, the > minimum window can be created is 100x100. Probably this is the reason you have > failed test on win trybot. Done. Made it bigger. You're such a savior Jian! I was just fretting about having to debug this windows failure :-) BTW, I'm using different sizes in the tests to cover for the case where bugs could be masked because all windows are the same size. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:419: CreatePanel("PanelTest2", gfx::Rect(0, 0, 110, 110)); On 2011/08/23 17:14:16, jennb wrote: > PanelTest3 Done. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:568: RestoreWorkArea(); On 2011/08/23 17:27:12, jianli wrote: > No need to restore work area. Done. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_manager.cc:11: #include "base/threading/platform_thread.h" On 2011/08/23 17:27:12, jianli wrote: > Why is this include needed? Done. Removed. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_manager.h:87: static int horizontal_spacing(); On 2011/08/23 17:14:16, jennb wrote: > Need comment explaining what this is. Done. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_manager.h:87: static int horizontal_spacing(); On 2011/08/23 17:27:12, jianli wrote: > Since this is only needed by the test, you should not make it accessible to > everyone. Either try to use UNIT_TEST to guard it or move it to private and > expose it to FRIEND test. This is private. Can't use UNIT_TEST because the defintion needs to live in the .cc file as the variable we want is in an anonymous namespace.
http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:174: panels[i]->Close(); On 2011/08/23 18:05:10, prasadt wrote: > On 2011/08/23 17:14:16, jennb wrote: > > Is it safe to modify the vector as you're iterating through it? > > We're not modifying the vector, rather the object stored in the vector. The > operation we're doing is safe. Closing a panel could remove the panel from PanelManager's panels_ vector. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:318: // **** Tests with a single panel. On 2011/08/23 18:05:10, prasadt wrote: > On 2011/08/23 17:14:16, jennb wrote: > > Not a big fan of extra **** or ==== in the comments... > > Sure. They make sense here though. > > > Why not create multiple > > tests - OnePanelDrag, TwoPanelsDrag, ThreePanelsDrag? > > It'll make the tests much slower as we start and shutdown the browser for each > test. What about subroutines then for testing one panel, two panels and three panels parts?
One general comment: is it possible to test for 1, 2, 3 and 4 panels in more
automatic way? Something like:
for (int i = 0; i < max_panels; ++i) {
// Create the panel.
...
// Test dragging being initiated from every panel.
for (int j = 0; j < i; ++j) {
int drag_start_x = panels[j]->GetBounds().x();
// Test dragging with small delta.
...
// Test dragging with small delta and then cancelling the drag.
...
// Test dragging to the right.
for (int k = 0; k <= j; ++k) {
if (k == j) continue;
// Compute delta.
int delta = panels[k]->GetBounds().right() - drag_start_x;
// Test dragging by delta.
// Test dragging by delta and cancelling the drag.
}
}
On 2011/08/23 18:12:52, jianli wrote:
> One general comment: is it possible to test for 1, 2, 3 and 4 panels in more
> automatic way? Something like:
> for (int i = 0; i < max_panels; ++i) {
> // Create the panel.
> ...
>
> // Test dragging being initiated from every panel.
> for (int j = 0; j < i; ++j) {
> int drag_start_x = panels[j]->GetBounds().x();
>
> // Test dragging with small delta.
> ...
>
> // Test dragging with small delta and then cancelling the drag.
> ...
>
> // Test dragging to the right.
> for (int k = 0; k <= j; ++k) {
> if (k == j) continue;
>
> // Compute delta.
> int delta = panels[k]->GetBounds().right() - drag_start_x;
>
> // Test dragging by delta.
>
> // Test dragging by delta and cancelling the drag.
> }
>
> }
That's sort of how I had the code before this CL and I spent more time than I
should trying to keep the structure and make the test generic and even had it
working that way. What I found is that I end up with multiple if statements for
each different condition making the test code complex and hard to follow. And
it also makes it difficult to figure what exactly is getting tested.
http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:174: panels[i]->Close(); On 2011/08/23 18:09:31, jennb wrote: > On 2011/08/23 18:05:10, prasadt wrote: > > On 2011/08/23 17:14:16, jennb wrote: > > > Is it safe to modify the vector as you're iterating through it? > > > > We're not modifying the vector, rather the object stored in the vector. The > > operation we're doing is safe. > > Closing a panel could remove the panel from PanelManager's panels_ vector. Good point. Its probably not closing all the panels the way it is. Fixed it by getting a copy instead of a reference to the vector. http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:318: // **** Tests with a single panel. On 2011/08/23 18:09:31, jennb wrote: > On 2011/08/23 18:05:10, prasadt wrote: > > On 2011/08/23 17:14:16, jennb wrote: > > > Not a big fan of extra **** or ==== in the comments... > > > > Sure. They make sense here though. > > > > > Why not create multiple > > > tests - OnePanelDrag, TwoPanelsDrag, ThreePanelsDrag? > > > > It'll make the tests much slower as we start and shutdown the browser for each > > test. > > What about subroutines then for testing one panel, two panels and three panels > parts? I don't see a net win. The variable declarations at the top will need to be repeated. And then the creation of panels has to be reconciled. Either each routing will open and close the panels or each successive routine makes an assumption that the previous routine already opened n-1 panels. I'd much rather keep it the way it is.
http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/1/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_browsertest.cc:318: // **** Tests with a single panel. On 2011/08/23 18:26:25, prasadt wrote: > On 2011/08/23 18:09:31, jennb wrote: > > On 2011/08/23 18:05:10, prasadt wrote: > > > On 2011/08/23 17:14:16, jennb wrote: > > > > Not a big fan of extra **** or ==== in the comments... > > > > > > Sure. They make sense here though. > > > > > > > Why not create multiple > > > > tests - OnePanelDrag, TwoPanelsDrag, ThreePanelsDrag? > > > > > > It'll make the tests much slower as we start and shutdown the browser for > each > > > test. > > > > What about subroutines then for testing one panel, two panels and three panels > > parts? > > I don't see a net win. The variable declarations at the top will need to be > repeated. And then the creation of panels has to be reconciled. Either each > routing will open and close the panels or each successive routine makes an > assumption that the previous routine already opened n-1 panels. > > I'd much rather keep it the way it is. Removed the special markers and replaced with scoping and indentation to separate out sections of code.
LGTM pending Jian's review. http://codereview.chromium.org/7706027/diff/8002/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/8002/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:166: std::vector<Panel*> panels = PanelManager::GetInstance()->panels_; Add a TODO here for dimich. When he makes Close() not call panel manager to remove panels immediately, it will be safe to use the actual vector of panels again. http://codereview.chromium.org/7706027/diff/8002/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:166: std::vector<Panel*> panels = PanelManager::GetInstance()->panels_; Can you use the Panels (typedef inside PanelManager) here? This test is a friend class of PanelManager so maybe you have access to that typedef.
http://codereview.chromium.org/7706027/diff/8002/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/8002/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:166: std::vector<Panel*> panels = PanelManager::GetInstance()->panels_; On 2011/08/23 21:26:48, jennb wrote: > Add a TODO here for dimich. When he makes Close() not call panel manager to > remove panels immediately, it will be safe to use the actual vector of panels > again. Does it matter? And worse we'd be be relying on that implementation detail when we don't have to. http://codereview.chromium.org/7706027/diff/8002/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:166: std::vector<Panel*> panels = PanelManager::GetInstance()->panels_; On 2011/08/23 21:26:48, jennb wrote: > Can you use the Panels (typedef inside PanelManager) here? This test is a > friend class of PanelManager so maybe you have access to that typedef. I'd rather just use the vector so we have it right there for someone to see instead of having to go to the header to figure what it is.
Jian - Please look at the changes I uploaded to fix the windows failures. Tests now pass on my windows machine. Hopefully the bots will agree with that.
Another test case to consider for 2 panels: Drag 1 panel either left or right to cause the shuffle, but do not end the drag. Then continue the drag in the reverse direction to shuffle back. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:165: void CloseAllPanels() { This helper function is not needed. You can call PanelManager::GetInstance()->RemoveAll() instead. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:195: enum DragAction { Please call it as DragAcitonMasks. Please comment that DRAG_ACTION_FINISH and DRAG_ACTION_CANCEL cannot be set simultaneously. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:204: std::vector<int>expected_delta_x_after_drag, Please add space between type and value. Also, please add "const &". http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:205: std::vector<int>expected_delta_x_after_finish, ditto. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:206: std::vector<gfx::Rect> initial_bounds, ditto. Also, I think it would better to name it as expected_bounds_after_cancel. Then you do not need to explain the difference between test_begin_bounds and initial_bounds. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:208: std::vector<Panel*> panels = PanelManager::GetInstance()->panels_; Better to expose panels_ as getter with UNIT_TEST guard. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:221: std::vector<gfx::Rect> test_begin_bounds = GetAllPanelsBounds(); If we rename initial_bounds to expected_bounds_after_cancel, test_begin_bounds could be renamed to start_bounds, for simplicity. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:239: // Compare against expected bounds. This comment could be moved down for the next line of code. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:241: for (size_t i = 0; i < panels.size(); ++i) { Can we have 2 helper functions for checking the expected bounds like the following: void CheckPanelBounds(const std::vector<gfx::Rect>& expected_bounds); void CheckPanelBoundsWithDeltas(const std::vector<int>& expected_deltas); http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:314: int small_delta = 10; This could be defined as "static const". http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:315: int big_delta = 70; ditto. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:316: int bigger_delta = 120; ditto. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:317: int biggest_delta = 200; ditto. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:331: TestDragging(-big_delta, 0, 0, expected_delta_x_after_drag, Maybe we can call 2nd argument '0' as zero_delta. Otherwise it is quite easy to have it confused with 3rd argument. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:362: // Tests with two panels. Probably better to organized like the following, for better readability: // Tests with two panels. CreatePanel("PanelTest2", gfx::Rect(0, 0, 120, 120)); // Drag left, small delta, expect no shuffle. { ... } // Drag right panel i.e index 0, towards left, big delta, expect shuffle. { ... } ... http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:378: expected_delta_x_after_finish[0] = expected_delta_x_after_finish[0] = initial_bounds[1].x() - initial_bounds[0].x(); http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:382: expected_delta_x_after_drag[1] = expected_delta_x_after_drag[1] = initial_bounds[0].right() - initial_bounds[1].width(); For test cases that involve shuffling only 2 panels, we could figure out the expected positions without knowing about horizontal_spacing. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:432: // Drag left most panel to become right most with two shuffles. left-most, right-most http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:435: // First drag and shuffle. Please explain what first drag is about, like: // Firstly drag the left-most panel towards right, without ending or cancelling it. Expect shuffle. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:446: // There is no delta after finish as drag is done yet. as drag is not done yet. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:450: // Second drag and shuffle. We finish the drag here. The drag index ditto. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_manager.cc:11: #include "base/threading/platform_thread.h" Please remove this include. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_manager.h:88: static int horizontal_spacing(); Can you make it public and add UNIT_TEST guard for it? You can move the definition of constant to this header file. Also, do not make it static method. Use PanelManager::GetInstance()->horizontal_spacing() to access it in the calling code, for consistency reason.
Jian - I addressed/answered all your comments. As for the additional test you suggested, I think its reasonably covered between the drag left, drag right and multi-drag scenarios. I think we have good coverage now. If there is something important missing, I'll add a TODO for later. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:165: void CloseAllPanels() { On 2011/08/24 20:23:24, jianli wrote: > This helper function is not needed. You can call > PanelManager::GetInstance()->RemoveAll() instead. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:195: enum DragAction { On 2011/08/24 20:23:24, jianli wrote: > Please call it as DragAcitonMasks. Please comment that DRAG_ACTION_FINISH and > DRAG_ACTION_CANCEL cannot be set simultaneously. I like the current name better, so leaving it that way. Added the comment though it feels too much commenting. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:204: std::vector<int>expected_delta_x_after_drag, On 2011/08/24 20:23:24, jianli wrote: > Please add space between type and value. Also, please add "const &". Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:205: std::vector<int>expected_delta_x_after_finish, On 2011/08/24 20:23:24, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:206: std::vector<gfx::Rect> initial_bounds, On 2011/08/24 20:23:24, jianli wrote: > ditto. > > Also, I think it would better to name it as expected_bounds_after_cancel. Then > you do not need to explain the difference between test_begin_bounds and > initial_bounds. I don't believe the name change will make it any easier for someone reading the code for the first time. I'm sticking with initial_bounds as that makes more sense to me. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:208: std::vector<Panel*> panels = PanelManager::GetInstance()->panels_; On 2011/08/24 20:23:24, jianli wrote: > Better to expose panels_ as getter with UNIT_TEST guard. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:221: std::vector<gfx::Rect> test_begin_bounds = GetAllPanelsBounds(); On 2011/08/24 20:23:24, jianli wrote: > If we rename initial_bounds to expected_bounds_after_cancel, test_begin_bounds > could be renamed to start_bounds, for simplicity. Sticking with my current names as explained earlier. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:239: // Compare against expected bounds. On 2011/08/24 20:23:24, jianli wrote: > This comment could be moved down for the next line of code. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:241: for (size_t i = 0; i < panels.size(); ++i) { On 2011/08/24 20:23:24, jianli wrote: > Can we have 2 helper functions for checking the expected bounds like the > following: > void CheckPanelBounds(const std::vector<gfx::Rect>& expected_bounds); > void CheckPanelBoundsWithDeltas(const std::vector<int>& expected_deltas); Leaving it the way it is. The function is reasonably sized and straightforward. I don't believe helper functions here will improve it. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:314: int small_delta = 10; On 2011/08/24 20:23:24, jianli wrote: > This could be defined as "static const". Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:315: int big_delta = 70; On 2011/08/24 20:23:24, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:316: int bigger_delta = 120; On 2011/08/24 20:23:24, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:317: int biggest_delta = 200; On 2011/08/24 20:23:24, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:331: TestDragging(-big_delta, 0, 0, expected_delta_x_after_drag, On 2011/08/24 20:23:24, jianli wrote: > Maybe we can call 2nd argument '0' as zero_delta. Otherwise it is quite easy to > have it confused with 3rd argument. Done. I already have a vector I call zero_delta, changed that to zero_deltas. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:362: // Tests with two panels. On 2011/08/24 20:23:24, jianli wrote: > Probably better to organized like the following, for better readability: > // Tests with two panels. > CreatePanel("PanelTest2", gfx::Rect(0, 0, 120, 120)); > > // Drag left, small delta, expect no shuffle. > { > ... > } > > // Drag right panel i.e index 0, towards left, big delta, expect shuffle. > { > ... > } > > ... The current organization sounds about right to me. Leaving it the way it is. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:378: expected_delta_x_after_finish[0] = On 2011/08/24 20:23:24, jianli wrote: > expected_delta_x_after_finish[0] = > initial_bounds[1].x() - initial_bounds[0].x(); Just keeping it consistent across all tests. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:382: expected_delta_x_after_drag[1] = On 2011/08/24 20:23:24, jianli wrote: > expected_delta_x_after_drag[1] = > initial_bounds[0].right() - initial_bounds[1].width(); > > For test cases that involve shuffling only 2 panels, we could figure out the > expected positions without knowing about horizontal_spacing. ditto. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:432: // Drag left most panel to become right most with two shuffles. On 2011/08/24 20:23:24, jianli wrote: > left-most, right-most Done. But replaced with leftmost and rightmost which are actual words. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:435: // First drag and shuffle. On 2011/08/24 20:23:24, jianli wrote: > Please explain what first drag is about, like: > // Firstly drag the left-most panel towards right, without ending or > cancelling it. Expect shuffle. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:446: // There is no delta after finish as drag is done yet. On 2011/08/24 20:23:24, jianli wrote: > as drag is not done yet. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:450: // Second drag and shuffle. We finish the drag here. The drag index On 2011/08/24 20:23:24, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_manager.cc (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_manager.cc:11: #include "base/threading/platform_thread.h" On 2011/08/24 20:23:24, jianli wrote: > Please remove this include. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_manager.h:88: static int horizontal_spacing(); On 2011/08/24 20:23:24, jianli wrote: > Can you make it public and add UNIT_TEST guard for it? You can move the > definition of constant to this header file. > > Also, do not make it static method. Use > PanelManager::GetInstance()->horizontal_spacing() to access it in the calling > code, for consistency reason. I'd rather not do this change. If we move one definition, we have to move others too to keep it consistent but no one else needs them, so they really belong in the cc file. Also it makes more sense for this function to be a static because its just returning a constant.
http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:206: std::vector<gfx::Rect> initial_bounds, On 2011/08/24 21:29:17, prasadt wrote: > On 2011/08/24 20:23:24, jianli wrote: > > ditto. > > > > Also, I think it would better to name it as expected_bounds_after_cancel. Then > > you do not need to explain the difference between test_begin_bounds and > > initial_bounds. > > I don't believe the name change will make it any easier for someone reading the > code for the first time. I'm sticking with initial_bounds as that makes more > sense to me. initial_bounds passed to this function is only used to check if the expected bounds after canceling the drag is same as the original bounds. So naming it as expected_bounds_after_cancel serves this purpose and improves the readability and thus you can remove the lengthy comment below. Otherwise, it took me quite long time to figure out what you want to do here. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:241: for (size_t i = 0; i < panels.size(); ++i) { On 2011/08/24 21:29:17, prasadt wrote: > On 2011/08/24 20:23:24, jianli wrote: > > Can we have adding 2 helper functions for checking the expected bounds like the > > following: > > void CheckPanelBounds(const std::vector<gfx::Rect>& expected_bounds); > > void CheckPanelBoundsWithDeltas(const std::vector<int>& expected_deltas); > > Leaving it the way it is. > The function is reasonably sized and straightforward. I don't believe helper > functions here will improve it. I still think 2 helper functions will simplify the code here and improve the readability. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:362: // Tests with two panels. On 2011/08/24 21:29:17, prasadt wrote: > On 2011/08/24 20:23:24, jianli wrote: > > Probably better to organized like the following, for better readability: > > // Tests with two panels. > > CreatePanel("PanelTest2", gfx::Rect(0, 0, 120, 120)); > > > > // Drag left, small delta, expect no shuffle. > > { > > ... > > } > > > > // Drag right panel i.e index 0, towards left, big delta, expect shuffle. > > { > > ... > > } > > > > ... > > The current organization sounds about right to me. Leaving it the way it is. Can you at least wrap all the codes for one test case into nested block, just like what you did for testing 3 panels? This will make us easily identify the scope of any test. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_manager.h:88: static int horizontal_spacing(); On 2011/08/24 21:29:17, prasadt wrote: > On 2011/08/24 20:23:24, jianli wrote: > > Can you make it public and add UNIT_TEST guard for it? You can move the > > definition of constant to this header file. > > > > Also, do not make it static method. Use > > PanelManager::GetInstance()->horizontal_spacing() to access it in the calling > > code, for consistency reason. > > I'd rather not do this change. If we move one definition, we have to move > others too to keep it consistent but no one else needs them, so they really > belong in the cc file. > > Also it makes more sense for this function to be a static because its just > returning a constant. We're trying to avoid accessing private methods and members directly from the testing code. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:514: // will shuffle middle panel to leftmost and rightmost to middle. Please move all the codes for this case to a nested block, just like what you did for the test case above. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:538: // Drag rightmost panel to become the leftmost in a single drag. This ditto. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:562: // Drag rightmost panel to become the leftmost in a single drag. Then ditto. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:59: Panels panels() { return panels_; } Please define it as: const Panels& panels() const { return panels_; }
Jian - Please take another look. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:206: std::vector<gfx::Rect> initial_bounds, On 2011/08/24 22:04:02, jianli wrote: > On 2011/08/24 21:29:17, prasadt wrote: > > On 2011/08/24 20:23:24, jianli wrote: > > > ditto. > > > > > > Also, I think it would better to name it as expected_bounds_after_cancel. > Then > > > you do not need to explain the difference between test_begin_bounds and > > > initial_bounds. > > > > I don't believe the name change will make it any easier for someone reading > the > > code for the first time. I'm sticking with initial_bounds as that makes more > > sense to me. > > initial_bounds passed to this function is only used to check if the expected > bounds after canceling the drag is same as the original bounds. So naming it as > expected_bounds_after_cancel serves this purpose and improves the readability > and thus you can remove the lengthy comment below. Otherwise, it took me quite > long time to figure out what you want to do here. Done. Leaving the comment as I think its still useful. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:241: for (size_t i = 0; i < panels.size(); ++i) { On 2011/08/24 22:04:02, jianli wrote: > On 2011/08/24 21:29:17, prasadt wrote: > > On 2011/08/24 20:23:24, jianli wrote: > > > Can we have adding 2 helper functions for checking the expected bounds like > the > > > following: > > > void CheckPanelBounds(const std::vector<gfx::Rect>& expected_bounds); > > > void CheckPanelBoundsWithDeltas(const std::vector<int>& expected_deltas); > > > > Leaving it the way it is. > > The function is reasonably sized and straightforward. I don't believe helper > > functions here will improve it. > > I still think 2 helper functions will simplify the code here and improve the > readability. Done. http://codereview.chromium.org/7706027/diff/7004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:362: // Tests with two panels. On 2011/08/24 22:04:02, jianli wrote: > On 2011/08/24 21:29:17, prasadt wrote: > > On 2011/08/24 20:23:24, jianli wrote: > > > Probably better to organized like the following, for better readability: > > > // Tests with two panels. > > > CreatePanel("PanelTest2", gfx::Rect(0, 0, 120, 120)); > > > > > > // Drag left, small delta, expect no shuffle. > > > { > > > ... > > > } > > > > > > // Drag right panel i.e index 0, towards left, big delta, expect shuffle. > > > { > > > ... > > > } > > > > > > ... > > > > The current organization sounds about right to me. Leaving it the way it is. > > Can you at least wrap all the codes for one test case into nested block, just > like what you did for testing 3 panels? This will make us easily identify the > scope of any test. Done. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:514: // will shuffle middle panel to leftmost and rightmost to middle. On 2011/08/24 22:04:03, jianli wrote: > Please move all the codes for this case to a nested block, just like what you > did for the test case above. Done. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:538: // Drag rightmost panel to become the leftmost in a single drag. This On 2011/08/24 22:04:03, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:562: // Drag rightmost panel to become the leftmost in a single drag. Then On 2011/08/24 22:04:03, jianli wrote: > ditto. Done. http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/14001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:59: Panels panels() { return panels_; } On 2011/08/24 22:04:03, jianli wrote: > Please define it as: > const Panels& panels() const { return panels_; } Done.
LGTM. Just a few small issues. Please check win trybot result to make sure it passes on win. http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:210: } Please add an empty line. http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:59: const Panels& panels() { return panels_; } Please append const modifier. http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:92: // Horizontal spacing between panels. Used for unit testing. Please remove this comment.
Address issues and fixed the windows test failures. PanelManager::RemoveAll is modifying the vector while iterating over it which is causing a crash on windows. Fixed it and verified the fix on windows. Committing the change. http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:210: } On 2011/08/25 17:22:26, jianli wrote: > Please add an empty line. Done. http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_manager.h (right): http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:59: const Panels& panels() { return panels_; } On 2011/08/25 17:22:26, jianli wrote: > Please append const modifier. Done. http://codereview.chromium.org/7706027/diff/19001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_manager.h:92: // Horizontal spacing between panels. Used for unit testing. On 2011/08/25 17:22:26, jianli wrote: > Please remove this comment. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
