Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6931039:
Add MenuItemView API to add and remove items at a particular index. (Closed)
|
Created:
9 years, 7 months ago by rhashimoto Modified:
9 years, 7 months ago CC:
chromium-reviews, Emmanuel Saint-loubert-Bié Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd MenuItemView API to add and remove items at a particular index.
The ChromeOS network menu needs to be able to change menu items
dynamically. In order to convert this menu from Menu2 to MenuItemView
it is cleanest to let MenuItemView own the code to modify the items.
BUG=none
TEST=included
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85313
Patch Set 1 #Patch Set 2 : Use CreateEventTask, fix Windows compile error. #Patch Set 3 : Fixed another spurious OVERRIDE. #
Total comments: 19
Patch Set 4 : Remove debugging delay parameter. #Patch Set 5 : Trial fix for enlarged menu flakiness. #Patch Set 6 : Implement testing workaround for GTK race. #Patch Set 7 : Rework based on sky's recommendations. #Patch Set 8 : Rebase to trunk. #
Total comments: 12
Patch Set 9 : Moved test to chrome/browser/ui/views. Implemented reviewer recommendations. #Patch Set 10 : Fixed indentation in test code. #
Messages
Total messages: 17 (0 generated)
This is a proposal to add AddMenuItemAt() and RemoveMenuItemAt() methods to MenuItemView. There is an accompanying interactive_ui_tests test, which includes checking if these methods work properly when a menu or submenu is open and items are added or removed. The test works properly with delays between steps but not at full speed (e.g. RunMenuAt() doesn't always get the menu up before the automation click) so I've marked all tests flaky for now. Is there a good way of dealing with these flakiness issues? I considered adding convenience API for different kinds of menu items, like there are for AppendMenuItem*. It wasn't clear that we have so many varied dynamic menu items so I left them out for now. I can easily add them if desired. The behavior when a submenu is open and items are changed in the primary menu is to close the submenu. That was the existing behavior of MenuController::MenuChildrenChanged(). I've made a couple stabs in my local tree (not in this CL) to keep the submenu open if it's not deleted, but have not gotten repositioning quite right. Would that behavior be preferred? Thanks, Roy
Drive-by with testing comments (consult chrome/test/OWNERS if in doubt). http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... File chrome/test/menu_item_view_test.cc (right): http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:7: #include "chrome/browser/automation/ui_controls.h" Are those chrome includes the only reason why this file has been put in chrome/test? It seems generally bad to have the code under test in one place and the test somewhere else. If you really need something from there, we should rather move that lower in the file hierarchy instead of moving this test higher. http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:9: #include "chrome/test/testing_profile.h" Is this #include used? http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:10: #include "chrome/test/ui_test_utils.h" Is this #include used? http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:22: static const int kStepMilliseconds = 0; I don't think it's a good idea to check that in (while of course it may help with local debugging/development). The PostDelayedTasks make the code more complicated and less deterministic (another task _might_ execute), which is generally bad. http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:63: delete menu_; Why not a scoped_ptr then? Non-scoped memory management like this is the #1 source of leaks in tests.
Thanks for the helpful feedback, Paweł. I'll adopt most of your notes later, but I wanted to ask for follow-up advice on a couple. On 2011/05/06 08:11:03, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... > chrome/test/menu_item_view_test.cc:7: #include > "chrome/browser/automation/ui_controls.h" > Are those chrome includes the only reason why this file has been put in > chrome/test? It seems generally bad to have the code under test in one place and > the test somewhere else. If you really need something from there, we should > rather move that lower in the file hierarchy instead of moving this test higher. Ah, I was wondering about this. The code being tested is under src/views, so it didn't appear that I could put the test beside the code and make it an interactive_ui test. Would the proper place for it be src/chrome/test/interactive_ui/? > http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... > chrome/test/menu_item_view_test.cc:22: static const int kStepMilliseconds = 0; > I don't think it's a good idea to check that in (while of course it may help > with local debugging/development). The PostDelayedTasks make the code more > complicated and less deterministic (another task _might_ execute), which is > generally bad. It looks like the implementation of PostTask is the same as PostDelayedTask with a 0 delay, so I don't think it should change determinism. I acknowledge that it makes the code more complicated. My thought was that leaving the hook for the delay in place would make it easier for a future developer to test changes. It made it easier for me in development and seemed a bit of a waste to remove it. The tests also seem to be flaky (I'm guessing because of a race between display and input) and the hook could be used to distinguish a flake from a real failure. I'm willing to remove the delay but I wanted to float those points first. Thanks, Roy
bookmark_bar_view_test runs at full speed. You should investigate what is different about your tests that cause it not to work correctly. http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... File chrome/test/menu_item_view_test.cc (right): http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:37: // MenuItemView prevents repeated activation of of a menu by 'of of' -> of http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:91: virtual void Click(views::View* view,Task* next) { 'view,task' -> 'view, task' http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_con... File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_con... views/controls/menu/menu_controller.cc:1351: SetSelection(item, SELECTION_OPEN_SUBMENU | SELECTION_UPDATE_IMMEDIATELY); This isn't right. It forces the selection to be what changed. What if item wasn't even open? http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... File views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.cc:280: { { on previous line. http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.cc:292: removed_items_.push_back(item); Why do we need to do this? BookmarkMenuController removes menu items. Are you wanting to use this to remove submenus? http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.cc:722: for (size_t i = 0; i < removed_items_.size(); ++i) STLDeleteElements in each place instead of this method. http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... File views/controls/menu/menu_item_view.h (right): http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.h:139: // ChildrenChanged() or DeleteRemovedItems() should be called after DeleteRemovedItems isn't public, so folks have to call ChildrenChanged after using this.
On 2011/05/06 16:04:34, rhashimoto wrote: > Ah, I was wondering about this. The code being tested is under src/views, so it > didn't appear that I could put the test beside the code and make it an > interactive_ui test. Would the proper place for it be > src/chrome/test/interactive_ui/? No, going deeper in chrome/test isn't what I meant. If the tested code is under src/views, please try to get your test into src/views. If you need to depend on something in chrome, move that something out of chrome into a place that can be used in src/views. Scott or Ben Goodger can probably help you with finding such a place. I'm not very familiar with views. > http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... > > chrome/test/menu_item_view_test.cc:22: static const int kStepMilliseconds = 0; > > I don't think it's a good idea to check that in (while of course it may help > > with local debugging/development). The PostDelayedTasks make the code more > > complicated and less deterministic (another task _might_ execute), which is > > generally bad. > > It looks like the implementation of PostTask is the same as PostDelayedTask with > a 0 delay, so I don't think it should change determinism. Possibly, but it's suspicious. It might change in the future. By the way I didn't know that, thanks for looking at that code. > I acknowledge that it makes the code more complicated. My thought was that > leaving the hook for the delay in place would make it easier for a future > developer to test changes. It made it easier for me in development and seemed a > bit of a waste to remove it. The tests also seem to be flaky (I'm guessing > because of a race between display and input) and the hook could be used to > distinguish a flake from a real failure. Hehe, we have another issue then. Are you going to check in a test that is already known to be flaky? Could you post something more about the issue? I think we should fix it - if it's flaky the failures are going to be ignored (by FLAKY or DISABLED prefix) and you'll get a bug about that. The main point is that checking in a test whose failures we're going to ignore because of the flakiness isn't really effective. > I'm willing to remove the delay but I wanted to float those points first. If that can help you with debugging the flake described above, feel free to remove it only after the root cause of that flakiness is identified. Anyway, yeah, I'd like this debugging code to get removed before checking in.
On 2011/05/06 18:56:52, Paweł Hajdan Jr. wrote: > If the tested code is under > src/views, please try to get your test into src/views. If you need to depend on > something in chrome, move that something out of chrome into a place that can be > used in src/views. Scott or Ben Goodger can probably help you with finding such > a place. I'm not very familiar with views. I will check with Scott for a better place to put the test. > Hehe, we have another issue then. Are you going to check in a test that is > already known to be flaky? Could you post something more about the issue? I > think we should fix it - if it's flaky the failures are going to be ignored (by > FLAKY or DISABLED prefix) and you'll get a bug about that. There are two sources of flakiness with GTK. The first may be limited to my local machine - I have not observed it on a trybot (yet) and I see the existing BookmarkBarViewTest flake out on my machine with the same error. The second occurs when trying to click on a menu that had items inserted. If the click is outside the original bounds of the menu (before insertion) sometimes the menu is dismissed without selecting an item. This appears to be a race in GTK. When the menu size is updated, it sets new bounds on the Widget. The new size notification comes back via gtk_container_idle_sizer(), which doesn't run if there are higher priority tasks in the queue. If the click is posted before the size updates then I get a failure. I added a workaround to the test code to directly update the menu bounds, essentially short-circuiting the notification. This seems to be working. > If that can help you with debugging the flake described above, feel free to > remove it only after the root cause of that flakiness is identified. Anyway, > yeah, I'd like this debugging code to get removed before checking in. I've removed all the delays (and unneeded headers). I have not yet addressed Scott's issues in the non-test code. Thanks, Roy
On 2011/05/10 00:20:33, rhashimoto wrote: > There are two sources of flakiness with GTK. The first may be limited to my > local machine - I have not observed it on a trybot (yet) and I see the existing > BookmarkBarViewTest flake out on my machine with the same error. Do you have some more details about this? If you're able to repro locally, it's obviously better to fix it for both tests rather than putting even more _known_ flakiness into the tree (someone else might then fix it in one place but not the other, and so on). > The second occurs when trying to click on a menu that had items inserted. If > the click is outside the original bounds of the menu (before insertion) > sometimes the menu is dismissed without selecting an item. > > This appears to be a race in GTK. When the menu size is updated, it sets new > bounds on the Widget. The new size notification comes back via > gtk_container_idle_sizer(), which doesn't run if there are higher priority tasks > in the queue. If the click is posted before the size updates then I get a > failure. > > I added a workaround to the test code to directly update the menu bounds, > essentially short-circuiting the notification. This seems to be working. I see. I wonder - maybe the test tries to be too "realistic" then? If you simulate OS-level clicks, it's generally known to be very fragile and prone to problems like the above. Are you sure you _need_ that kind of simulation? Would it be possible to invoke some method rather than simulating the click? Note that a test that might seem less realistic but is very solid is arguably better than a test that tries to be more realistic, but ends up being flaky or which has a lot of workarounds that can break in the future and make the code harder to understand (and less realistic too).
On 2011/05/10 07:35:12, Paweł Hajdan Jr. wrote: > On 2011/05/10 00:20:33, rhashimoto wrote: > > There are two sources of flakiness with GTK. The first may be limited to my > > local machine - I have not observed it on a trybot (yet) and I see the > existing > > BookmarkBarViewTest flake out on my machine with the same error. > > Do you have some more details about this? If you're able to repro locally, it's > obviously better to fix it for both tests rather than putting even more _known_ > flakiness into the tree (someone else might then fix it in one place but not the > other, and so on). There is an issue with interactive_ui_tests/ui_controls on linux/gtk where it may send events before widget is ready, and you may be hitting this issue. I can help you troubleshoot this so please let me know. > > The second occurs when trying to click on a menu that had items inserted. If > > the click is outside the original bounds of the menu (before insertion) > > sometimes the menu is dismissed without selecting an item. > > > > This appears to be a race in GTK. When the menu size is updated, it sets new > > bounds on the Widget. The new size notification comes back via > > gtk_container_idle_sizer(), which doesn't run if there are higher priority > tasks > > in the queue. If the click is posted before the size updates then I get a > > failure. > > > > I added a workaround to the test code to directly update the menu bounds, > > essentially short-circuiting the notification. This seems to be working. > > I see. I wonder - maybe the test tries to be too "realistic" then? If you > simulate OS-level clicks, it's generally known to be very fragile and prone to > problems like the above. Are you sure you _need_ that kind of simulation? Would > it be possible to invoke some method rather than simulating the click? Note that > a test that might seem less realistic but is very solid is arguably better than > a test that tries to be more realistic, but ends up being flaky or which has a > lot of workarounds that can break in the future and make the code harder to > understand (and less realistic too).
On 2011/05/10 07:58:33, oshima wrote: > There is an issue with interactive_ui_tests/ui_controls on linux/gtk where it > may send > events before widget is ready, and you may be hitting this issue. I can help > you troubleshoot > this so please let me know. That does sound like the problem I am seeing. What happens is that ui_controls::MoveMouseToCenterAndPress() moves the mouse to the center of the widget (the contents view passed to ViewEventTestBase) and then calls gdk_window_at_pointer() which returns NULL, meaning window not known to the application.
Tue, May 10, 2011 at 12:58 AM, <oshima@chromium.org> wrote: > On 2011/05/10 07:35:12, Paweł Hajdan Jr. wrote: >> >> On 2011/05/10 00:20:33, rhashimoto wrote: >> > There are two sources of flakiness with GTK. The first may be limited >> > to my >> > local machine - I have not observed it on a trybot (yet) and I see the >> existing >> > BookmarkBarViewTest flake out on my machine with the same error. > >> Do you have some more details about this? If you're able to repro locally, > > it's >> >> obviously better to fix it for both tests rather than putting even more > > _known_ >> >> flakiness into the tree (someone else might then fix it in one place but >> not > > the >> >> other, and so on). > > There is an issue with interactive_ui_tests/ui_controls on linux/gtk where > it > may send > events before widget is ready, and you may be hitting this issue. I can > help > you troubleshoot > this so please let me know. Oshima, is there some window event we could make all tests listen for before starting that would avoid this problem? -Scott >> > The second occurs when trying to click on a menu that had items >> > inserted. > > If >> >> > the click is outside the original bounds of the menu (before insertion) >> > sometimes the menu is dismissed without selecting an item. >> > >> > This appears to be a race in GTK. When the menu size is updated, it >> > sets > > new >> >> > bounds on the Widget. The new size notification comes back via >> > gtk_container_idle_sizer(), which doesn't run if there are higher >> > priority >> tasks >> > in the queue. If the click is posted before the size updates then I get >> > a >> > failure. >> > >> > I added a workaround to the test code to directly update the menu >> > bounds, >> > essentially short-circuiting the notification. This seems to be >> > working. > >> I see. I wonder - maybe the test tries to be too "realistic" then? If you >> simulate OS-level clicks, it's generally known to be very fragile and >> prone to >> problems like the above. Are you sure you _need_ that kind of simulation? > > Would >> >> it be possible to invoke some method rather than simulating the click? >> Note > > that >> >> a test that might seem less realistic but is very solid is arguably better > > than >> >> a test that tries to be more realistic, but ends up being flaky or which >> has a >> lot of workarounds that can break in the future and make the code harder >> to >> understand (and less realistic too). > > > > http://codereview.chromium.org/6931039/ >
On Tue, May 10, 2011 at 9:34 AM, Scott Violet <sky@chromium.org> wrote: > Tue, May 10, 2011 at 12:58 AM, <oshima@chromium.org> wrote: > > On 2011/05/10 07:35:12, Paweł Hajdan Jr. wrote: > >> > >> On 2011/05/10 00:20:33, rhashimoto wrote: > >> > There are two sources of flakiness with GTK. The first may be limited > >> > to my > >> > local machine - I have not observed it on a trybot (yet) and I see the > >> existing > >> > BookmarkBarViewTest flake out on my machine with the same error. > > > >> Do you have some more details about this? If you're able to repro > locally, > > > > it's > >> > >> obviously better to fix it for both tests rather than putting even more > > > > _known_ > >> > >> flakiness into the tree (someone else might then fix it in one place but > >> not > > > > the > >> > >> other, and so on). > > > > There is an issue with interactive_ui_tests/ui_controls on linux/gtk > where > > it > > may send > > events before widget is ready, and you may be hitting this issue. I can > > help > > you troubleshoot > > this so please let me know. > > Oshima, is there some window event we could make all tests listen for > before starting that would avoid this problem? > Yes, I was going to fix this, and I will this time :) - oshima > > -Scott > > >> > The second occurs when trying to click on a menu that had items > >> > inserted. > > > > If > >> > >> > the click is outside the original bounds of the menu (before > insertion) > >> > sometimes the menu is dismissed without selecting an item. > >> > > >> > This appears to be a race in GTK. When the menu size is updated, it > >> > sets > > > > new > >> > >> > bounds on the Widget. The new size notification comes back via > >> > gtk_container_idle_sizer(), which doesn't run if there are higher > >> > priority > >> tasks > >> > in the queue. If the click is posted before the size updates then I > get > >> > a > >> > failure. > >> > > >> > I added a workaround to the test code to directly update the menu > >> > bounds, > >> > essentially short-circuiting the notification. This seems to be > >> > working. > > > >> I see. I wonder - maybe the test tries to be too "realistic" then? If > you > >> simulate OS-level clicks, it's generally known to be very fragile and > >> prone to > >> problems like the above. Are you sure you _need_ that kind of > simulation? > > > > Would > >> > >> it be possible to invoke some method rather than simulating the click? > >> Note > > > > that > >> > >> a test that might seem less realistic but is very solid is arguably > better > > > > than > >> > >> a test that tries to be more realistic, but ends up being flaky or which > >> has a > >> lot of workarounds that can break in the future and make the code harder > >> to > >> understand (and less realistic too). > > > > > > > > http://codereview.chromium.org/6931039/ > > >
I have been testing this extensively and I've determined that this is relevant to the use of gtk_window_get_size and gtk_window_get_position. Here's the relevant note from the Gtk docs for gtk_window_get_size: Note 1: Nearly any use of this function creates a race condition, because the size of the window may change between the time that you get the size and the time that you perform some action assuming that size is the current size. To avoid race conditions, connect to "configure-event" on the window and adjust your size-dependent state to match the size delivered in the GdkEventConfigure. I have observed the same thing, that gdk_window_at_pointer returns NULL in FakeAMouseMotionEvent because the coordinates chosen for the pointer came from bogus window bounds. The bogus window bounds comes from gtk_window_get_position and gtk_window_get_position in WidgetGtk::GetClientAreaScreenBounds. If possible, I'd like to see the configure-event technique applied to fix this race condition.
Hi Scott - Do you have a recommendation on where the test should go in the tree (an interactive_ui test for MenuItemView)? I've tried to address your issues inline below. Thanks, Roy http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... File chrome/test/menu_item_view_test.cc (right): http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:37: // MenuItemView prevents repeated activation of of a menu by On 2011/05/06 16:17:00, sky wrote: > 'of of' -> of Done. http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:63: delete menu_; On 2011/05/06 08:11:03, Paweł Hajdan Jr. wrote: > Why not a scoped_ptr then? Non-scoped memory management like this is the #1 > source of leaks in tests. Done. http://codereview.chromium.org/6931039/diff/4002/chrome/test/menu_item_view_t... chrome/test/menu_item_view_test.cc:91: virtual void Click(views::View* view,Task* next) { On 2011/05/06 16:17:00, sky wrote: > 'view,task' -> 'view, task' Done. http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_con... File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_con... views/controls/menu/menu_controller.cc:1351: SetSelection(item, SELECTION_OPEN_SUBMENU | SELECTION_UPDATE_IMMEDIATELY); On 2011/05/06 16:17:00, sky wrote: > This isn't right. It forces the selection to be what changed. What if item > wasn't even open? You're right. I've changed this to check if the changed item is an ancestor of the selection (or *is* the selection). If yes, then the selection is moved to the changed item and repositioned. Otherwise nothing is done. http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... File views/controls/menu/menu_item_view.cc (right): http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.cc:280: { On 2011/05/06 16:17:00, sky wrote: > { on previous line. Done. http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.cc:292: removed_items_.push_back(item); On 2011/05/06 16:17:00, sky wrote: > Why do we need to do this? BookmarkMenuController removes menu items. Are you > wanting to use this to remove submenus? The use case is for the ChromiumOS network menu which asynchronously refreshes the list of available networks of various kinds, some on the top-level menu and some on a submenu. There is no anticipated case of adding or removing submenus while a menu is posted (it is needed when the menu is offline). When BookmarkMenuController removes items, it uses a similar approach of saving removed items in a container, calling ChildrenChanged(), and then deleting the items in the container. It does this with a method that removes multiple items at once (BookmarkMenuController::WillRemoveBookmarks), calling ChildrenChanged() and deleting items internally. As an alternative to this proposed API on MenuItemView, I could implement a bulk API like BookmarkMenuController (handling multiple adds/removes at once) or forego an API completely (have the network code manipulate menu items directly) if that is preferable. http://codereview.chromium.org/6931039/diff/4002/views/controls/menu/menu_ite... views/controls/menu/menu_item_view.cc:722: for (size_t i = 0; i < removed_items_.size(); ++i) On 2011/05/06 16:17:00, sky wrote: > STLDeleteElements in each place instead of this method. Done.
On Tue, May 10, 2011 at 10:24 AM, <wyck@chromium.org> wrote: > I have been testing this extensively and I've determined that this is > relevant > to the use of gtk_window_get_size and gtk_window_get_position. > > Here's the relevant note from the Gtk docs for gtk_window_get_size: > > Note 1: Nearly any use of this function creates a race condition, because > the > size of the window may change between the time that you get the size and > the > time that you perform some action assuming that size is the current size. > To > avoid race conditions, connect to "configure-event" on the window and > adjust > your size-dependent state to match the size delivered in the > GdkEventConfigure. > I have observed the same thing, that gdk_window_at_pointer returns NULL in > FakeAMouseMotionEvent because the coordinates chosen for the pointer came > from > bogus window bounds. The bogus window bounds > comes from gtk_window_get_position and gtk_window_get_position in > WidgetGtk::GetClientAreaScreenBounds. If possible, I'd like to see the configure-event technique applied to fix > this > race condition. Problem is that X is asynchronous, so so there is no guarantee that the window you just showed has the size and position that you specified when you send a event, even if there is no-one who tries to move/modify window. In this particular case, it's getting NULL window because menu sometimes gets default size (1x1) before it's resized to the specified size. And yes, solution is to synchronize using configure event. - oshima > > http://codereview.chromium.org/6931039/ >
For now put the test in chrome/browser/ui/views. It's not the best place, but your test can't live in views because it has dependencies on things in chrome. ViewEventTestBase has similar dependencies. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... File chrome/test/menu_item_view_test.cc (right): http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:33: MenuItemViewTestBase() : ViewEventTestBase(), wrap to next line and indent 4. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:177: MenuItemViewTestInsert() : last_command_(0), wrap and indent 4. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:321: private: indentation is off. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:491: private: indentation is off. http://codereview.chromium.org/6931039/diff/21002/views/controls/menu/menu_co... File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/6931039/diff/21002/views/controls/menu/menu_co... views/controls/menu/menu_controller.cc:1349: const MenuItemView* ancestor = state_.item; You should update if either state_.item or pending_state_.item matches. http://codereview.chromium.org/6931039/diff/21002/views/controls/menu/menu_co... views/controls/menu/menu_controller.cc:1354: OpenMenuImpl(item, false); You should only invoke OpenMenuItem if item has a submenu.
http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... File chrome/test/menu_item_view_test.cc (right): http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:33: MenuItemViewTestBase() : ViewEventTestBase(), On 2011/05/11 14:26:10, sky wrote: > wrap to next line and indent 4. Done. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:177: MenuItemViewTestInsert() : last_command_(0), On 2011/05/11 14:26:10, sky wrote: > wrap and indent 4. Done. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:321: private: On 2011/05/11 14:26:10, sky wrote: > indentation is off. Done. http://codereview.chromium.org/6931039/diff/21002/chrome/test/menu_item_view_... chrome/test/menu_item_view_test.cc:491: private: On 2011/05/11 14:26:10, sky wrote: > indentation is off. Done. http://codereview.chromium.org/6931039/diff/21002/views/controls/menu/menu_co... File views/controls/menu/menu_controller.cc (right): http://codereview.chromium.org/6931039/diff/21002/views/controls/menu/menu_co... views/controls/menu/menu_controller.cc:1349: const MenuItemView* ancestor = state_.item; On 2011/05/11 14:26:10, sky wrote: > You should update if either state_.item or pending_state_.item matches. Done. http://codereview.chromium.org/6931039/diff/21002/views/controls/menu/menu_co... views/controls/menu/menu_controller.cc:1354: OpenMenuImpl(item, false); On 2011/05/11 14:26:10, sky wrote: > You should only invoke OpenMenuItem if item has a submenu. Done, though I think we only get here if the item has added or removed children, which would imply a submenu is present. However, it is not a given that the submenu is open on entry to MenuChildrenChanged() so I removed the DCHECK for that.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
