Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450903002:
MacViews: Clear mouse handler when showing context menus. (Closed)
DescriptionMacViews: Clear mouse handler when showing context menus.
Currently the View that invokes a menu may incorrectly retain capture and
swallow subsequent mouse events. To fix we explicitly clear mouse handler when
showing context menus.
This same issue was previously fixed individually within ToolbarButton (r15)
and Combobox (r83617) before the common fix inside MenuController (r192688). On
MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa
which didn't contain the fix for this issue.
Move the fix to the common place that's shared between all the implementations
of MenuRunnerImplInterface.
BUG=659204
TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab;
Right-click the first tab and select Reopen Closed Tab; Click on the second tab.
Second tab should activate. See crbug.com/659204 for video.
Committed: https://crrev.com/c329b603044633b6ff0b2f17b2dfc5a1f1200ffa
Cr-Commit-Position: refs/heads/master@{#429673}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Moved fix to MenuRunner::RunMenuAt, remove redundant fixes. #
Total comments: 2
Patch Set 3 : Added test. #
Total comments: 10
Patch Set 4 : Removed duplicate code in test. #
Total comments: 4
Patch Set 5 : Fix review issues. #Patch Set 6 : Fix test on Windows. #Patch Set 7 : Fix test on both Mac and Windows. #
Messages
Total messages: 36 (18 generated)
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. BUG=659204 ========== to ========== MacViews: Clear mouse handler when showing context menus. BUG=659204 ==========
mblsha@yandex-team.ru changed reviewers: + sadrul@chromium.org, tapted@chromium.org
I've provided the reproduction steps + video: https://bugs.chromium.org/p/chromium/issues/detail?id=659204
tapted@chromium.org changed reviewers: + sky@chromium.org
+sky for his menu-code superpowers - but there are lots of callsites doing SetMouseHandler(nullptr) (e.g. in OnMenuClosed, as well as MenuController::Run()). I think we can delete them all and just have one at the start of MenuRunner::RunMenuAt (and thanks for the great BUG=, but we usually put a brief summary in the CL description as well per https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... . E.g. "Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix <describe fix>." https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right): https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:134: if (parent && parent->GetRootView()) above there's a DCHECK(parent), so there's no need to check |parent| here as well. And the RootView is only reset in Widget::~Widget(), so I don't think there's a need to check GetRootView either. However, MenuRunner::RunMenuAt(..) isn't as strict, so we may want to keep the checks if this fragment moves there. https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:135: parent->GetRootView()->SetMouseHandler(nullptr); This is just copied from menu_controller.cc - can we move this step and the comment into MenuRunner::RunMenuAt instead so that it's not duplicated? It's possible there's a reason not to. E.g. MenuRunnerImpl::RunMenuAt may exit early before invoking MenuController::Run, so the codepaths are not identical. sky@'s amazing review powers may see something I've missed, but this approach seems sensible if no other errors appear, e.g., in existing tests. But also this fragment appears in a bunch of places. The one in menu_controller.cc was added in r192688 There's another in combobox.cc that was "added" after that, in r223213. But actually it was moved from some code added in r84253. Well, actually before that even since that's also a move. So the fragment in combobox.cc is probably redundant with the code that appears in menu_controller.cc (which was added after) and we can delete it too. These fragments are also in OnMenuClosed handlers in toolbar_button.cc and browser.cc -- those can probably be deleted as well.
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. BUG=659204 ========== to ========== MacViews: Clear mouse handler when showing context menus. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ==========
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ========== to ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ==========
Updated Issue Description, hope it's better now. I wonder what's the best place to write a test for this would be: I think it should create a Widget with two Views, click the first one to show a Menu, dismiss the menu, click the second View and verify that it was clicked. Maybe menu_runner_unittest.cc? Clicks probably should be generated using ui::test::EventGenerator(). https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner_impl_cocoa.mm (right): https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner_impl_cocoa.mm:135: parent->GetRootView()->SetMouseHandler(nullptr); The fragment in browser.cc is from 2016, and it's for 'mash' which I'm not sure how to test. Others seem safe, so your proposal sounds good to me.
https://codereview.chromium.org/2450903002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/2450903002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.cc:63: if (parent && parent->GetRootView()) Can you clarify why you need need to move this here rather than in MenuController? Also, please provide test coverage.
https://codereview.chromium.org/2450903002/diff/20001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/2450903002/diff/20001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.cc:63: if (parent && parent->GetRootView()) On 2016/10/26 17:46:11, sky wrote: > Can you clarify why you need need to move this here rather than in > MenuController? Also, please provide test coverage. As tapted suggested in his first comment, this seems to be a common place among ToolbarButton, Combobox and the MenuController. It seems that we can remove all these hacks by fixing the problem in a common place they all use. Also added the test.
On 2016/10/27 15:05:29, themblsha wrote: > https://codereview.chromium.org/2450903002/diff/20001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner.cc (right): > > https://codereview.chromium.org/2450903002/diff/20001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner.cc:63: if (parent && parent->GetRootView()) > On 2016/10/26 17:46:11, sky wrote: > > Can you clarify why you need need to move this here rather than in > > MenuController? Also, please provide test coverage. > > As tapted suggested in his first comment, this seems to be a common place among > ToolbarButton, Combobox and the MenuController. It seems that we can remove all > these hacks by fixing the problem in a common place they all use. > > Also added the test. All the code calls into MenuController, right? So I don't understand why the code needs to move to MenuRunner?
https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.cc:66: return impl_->RunMenuAt(parent, button, bounds, anchor, run_types_); (answering for themblsha :), This line may call MenuRunnerImplCocoa::RunMenuAt(..) which is using the NSMenu menu controller (ui/base/cocoa/menu_contoller.mm), rather than a views::MenuController Patchset 1 copied the logic verbatim from menu_controller.cc to menu_runner_impl_cocoa.mm, but that code fragment was already scattered around a bunch of places, so I suggested we move it here at https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... . (maybe it's nicer to duplicate the code - sky will know!) (themblsha: It's generally good to capture stuff like this in the CL description - I don't want to write all your CL descriptions for you, so feel free to go into more detail. When describing a fix, it's good not just to say "how" but also "why" -- e.g. by moving the code in MenuController to MenuRunner we allow all implementors of MenuRunnerImplInterface to benefit from the fix. and, as earlier in the review for this CL and in other places, I keep pointing yandex folks to https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... . Please read it all. Specifically in this case "Long lines should be wrapped to 80 columns for easier log message viewing in terminals.") https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_unittest.cc (right): https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:258: // will be delivered to the correct view, and not to the one that shown the nit: shown -> showed (or `has shown` - English grammar is weird :/) https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:275: InitMenuRunner(MenuRunner::ASYNC); I think this will make a MenuRunnerImpl on Mac, so the test should already pass without your fix. Passing MenuRunner::CONTEXT_MENU would use MenuRunnerImplCocoa on Mac to test the bug being fixed. But then the menu runner won't be asynchronous - that makes testing harder. I think the coverage this provides is still good. The fix in menu_controller.cc was added in http://crrev.com/192688 without test coverage, so this helps resolve that (thanks!). To test the NSMenu flow, there are a bunch of tests in menu_runner_cocoa_unittest.mm, but they're testing at a lower level (no MenuRunner, just MenuRunnerImpls). I'm ok without extra stuff for MenuRunnerImplCocoa - moving the logic to MenuRunner is aligning the contract of what the MenuRunnerImpls need to fulfil. https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:279: event_count_view->AddPostTargetHandler(&consumer); everything above is copy-paste from WidgetDoesntTakeCapture . Also the |generator| below - is there a neat way to share it? Perhaps a `MenuRunnerWidgetTest` class https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:297: EXPECT_FALSE(runner->IsRunning());
On 2016/10/27 23:19:24, tapted wrote: > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner.cc (right): > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner.cc:66: return impl_->RunMenuAt(parent, > button, bounds, anchor, run_types_); > (answering for themblsha :), This line may call > > MenuRunnerImplCocoa::RunMenuAt(..) > > which is using the NSMenu menu controller (ui/base/cocoa/menu_contoller.mm), > rather than a views::MenuController My mistake, moving LGTM me. Make sure you address tapteds concerns though. > > Patchset 1 copied the logic verbatim from menu_controller.cc to > menu_runner_impl_cocoa.mm, but that code fragment was already scattered around a > bunch of places, so I suggested we move it here at > https://codereview.chromium.org/2450903002/diff/1/ui/views/controls/menu/menu... > . (maybe it's nicer to duplicate the code - sky will know!) > > (themblsha: It's generally good to capture stuff like this in the CL description > - I don't want to write all your CL descriptions for you, so feel free to go > into more detail. When describing a fix, it's good not just to say "how" but > also "why" -- e.g. by moving the code in MenuController to MenuRunner we allow > all implementors of MenuRunnerImplInterface to benefit from the fix. > > and, as earlier in the review for this CL and in other places, I keep pointing > yandex folks to > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... > . Please read it all. Specifically in this case "Long lines should be wrapped to > 80 columns for easier log message viewing in > terminals.") > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner_unittest.cc (right): > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_unittest.cc:258: // will be delivered to the > correct view, and not to the one that shown the > nit: shown -> showed (or `has shown` - English grammar is weird :/) > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_unittest.cc:275: > InitMenuRunner(MenuRunner::ASYNC); > I think this will make a MenuRunnerImpl on Mac, so the test should already pass > without your fix. Passing MenuRunner::CONTEXT_MENU would use MenuRunnerImplCocoa > on Mac to test the bug being fixed. But then the menu runner won't be > asynchronous - that makes testing harder. > > I think the coverage this provides is still good. The fix in menu_controller.cc > was added in http://crrev.com/192688 without test coverage, so this helps > resolve that (thanks!). > > To test the NSMenu flow, there are a bunch of tests in > menu_runner_cocoa_unittest.mm, but they're testing at a lower level (no > MenuRunner, just MenuRunnerImpls). I'm ok without extra stuff for > MenuRunnerImplCocoa - moving the logic to MenuRunner is aligning the contract of > what the MenuRunnerImpls need to fulfil. > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_unittest.cc:279: > event_count_view->AddPostTargetHandler(&consumer); > everything above is copy-paste from WidgetDoesntTakeCapture . Also the > |generator| below - is there a neat way to share it? > > Perhaps a `MenuRunnerWidgetTest` class > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner_unittest.cc:297: > EXPECT_FALSE(runner->IsRunning());
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ========== to ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. The work-around for this issue was already scattered throughout the code before the r192688 was committed. Move the fix to the place that's shared between all the implementations of MenuRunner. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ==========
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. The work-around for this issue was already scattered throughout the code before the r192688 was committed. Move the fix to the place that's shared between all the implementations of MenuRunner. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ========== to ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ==========
https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner.cc:66: return impl_->RunMenuAt(parent, button, bounds, anchor, run_types_); On 2016/10/27 23:19:24, tapted wrote: > and, as earlier in the review for this CL and in other places, I keep pointing > yandex folks to > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... > . Please read it all. Specifically in this case "Long lines should be wrapped to > 80 columns for easier log message viewing in > terminals.") Thanks for your tremendous patience with helping us improve the submitted CLs. We're not really coordinated, but personally I'll try to put more effort into my descriptions. 80 columns is understandable, but what about the TEST line? I left it in a single line because it was like this in the provided example. https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_unittest.cc (right): https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:258: // will be delivered to the correct view, and not to the one that shown the On 2016/10/27 23:19:24, tapted wrote: > nit: shown -> showed (or `has shown` - English grammar is weird :/) Done. https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:275: InitMenuRunner(MenuRunner::ASYNC); On 2016/10/27 23:19:24, tapted wrote: > I think this will make a MenuRunnerImpl on Mac, so the test should already pass > without your fix. Passing MenuRunner::CONTEXT_MENU would use MenuRunnerImplCocoa > on Mac to test the bug being fixed. But then the menu runner won't be > asynchronous - that makes testing harder. > > I think the coverage this provides is still good. The fix in menu_controller.cc > was added in http://crrev.com/192688 without test coverage, so this helps > resolve that (thanks!). > > To test the NSMenu flow, there are a bunch of tests in > menu_runner_cocoa_unittest.mm, but they're testing at a lower level (no > MenuRunner, just MenuRunnerImpls). I'm ok without extra stuff for > MenuRunnerImplCocoa - moving the logic to MenuRunner is aligning the contract of > what the MenuRunnerImpls need to fulfil. Yeah, it's non-Cocoa but tests a common code path now. https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:279: event_count_view->AddPostTargetHandler(&consumer); On 2016/10/27 23:19:24, tapted wrote: > everything above is copy-paste from WidgetDoesntTakeCapture . Also the > |generator| below - is there a neat way to share it? > > Perhaps a `MenuRunnerWidgetTest` class Thanks for the name :-) https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:297: On 2016/10/27 23:19:24, tapted wrote: > EXPECT_FALSE(runner->IsRunning()); Done.
lgtm % nits On 2016/10/29 13:47:19, themblsha wrote: > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > File ui/views/controls/menu/menu_runner.cc (right): > > https://codereview.chromium.org/2450903002/diff/40001/ui/views/controls/menu/... > ui/views/controls/menu/menu_runner.cc:66: return impl_->RunMenuAt(parent, > button, bounds, anchor, run_types_); > On 2016/10/27 23:19:24, tapted wrote: > > and, as earlier in the review for this CL and in other places, I keep pointing > > yandex folks to > > > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... > > . Please read it all. Specifically in this case "Long lines should be wrapped > to > > 80 columns for easier log message viewing in > > terminals.") > > Thanks for your tremendous patience with helping us improve the submitted CLs. > We're not really coordinated, but personally I'll try to put more effort into my > descriptions. > > 80 columns is understandable, but what about the TEST line? I left it in a > single line because it was like this in the provided example. Thanks! That's a really good CL description. The TEST= should be wrapped (I updated the contributing-code page - thanks for pointing that out). And it's missing a bit about the expected behaviour "Second tab should activate", e.g. TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. Second tab should activate. See crbug.com/659204 for video. Our QA team occasionally scans the commit history for these, and will follow the steps (if they're unclear they may ask on the BUG= for a clarification). https://codereview.chromium.org/2450903002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_unittest.cc (right): https://codereview.chromium.org/2450903002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:218: class MenuRunnerWidgetTest : public MenuRunnerTest { nit: brief comment like // Test harness that includes a parent Widget and View invoking the menu. https://codereview.chromium.org/2450903002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:260: Widget* widget_; nit: = nullptr;, same below. (although it's not critical now, it's hard to predict whether a future refactoring may alter SetUp(), and someone may assume this is initialized)
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. See crbug.com/659204 for video. ========== to ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. Second tab should activate. See crbug.com/659204 for video. ==========
Ah, I thought that TEST could be machine-readable. Thanks for clarification! https://codereview.chromium.org/2450903002/diff/60001/ui/views/controls/menu/... File ui/views/controls/menu/menu_runner_unittest.cc (right): https://codereview.chromium.org/2450903002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:218: class MenuRunnerWidgetTest : public MenuRunnerTest { On 2016/10/30 23:25:53, tapted wrote: > nit: brief comment like > // Test harness that includes a parent Widget and View invoking the menu. Done. https://codereview.chromium.org/2450903002/diff/60001/ui/views/controls/menu/... ui/views/controls/menu/menu_runner_unittest.cc:260: Widget* widget_; On 2016/10/30 23:25:53, tapted wrote: > nit: = nullptr;, same below. (although it's not critical now, it's hard to > predict whether a future refactoring may alter SetUp(), and someone may assume > this is initialized) Hopefully this would make possible future crashes simpler to debug, so added explicit intialization, thanks!
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2450903002/#ps80001 (title: "Fix review issues.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2450903002/#ps100001 (title: "Fix test on Windows.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2450903002/#ps120001 (title: "Fix test on both Mac and Windows.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. Second tab should activate. See crbug.com/659204 for video. ========== to ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. Second tab should activate. See crbug.com/659204 for video. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. Second tab should activate. See crbug.com/659204 for video. ========== to ========== MacViews: Clear mouse handler when showing context menus. Currently the View that invokes a menu may incorrectly retain capture and swallow subsequent mouse events. To fix we explicitly clear mouse handler when showing context menus. This same issue was previously fixed individually within ToolbarButton (r15) and Combobox (r83617) before the common fix inside MenuController (r192688). On MacViews the MenuController is not used, and we have the MenuRunnerImplCocoa which didn't contain the fix for this issue. Move the fix to the common place that's shared between all the implementations of MenuRunnerImplInterface. BUG=659204 TEST=Open Chromium, open two tabs with non-empty URLs; Close the second tab; Right-click the first tab and select Reopen Closed Tab; Click on the second tab. Second tab should activate. See crbug.com/659204 for video. Committed: https://crrev.com/c329b603044633b6ff0b2f17b2dfc5a1f1200ffa Cr-Commit-Position: refs/heads/master@{#429673} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c329b603044633b6ff0b2f17b2dfc5a1f1200ffa Cr-Commit-Position: refs/heads/master@{#429673} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
