Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2073783003:
Fixed bug where mash chrome crashes accessing ash::Shell with context menus (Closed)
Descriptionfixed issue 606368: early return from ProcessAcceleratorWhileMenuShowing() if chrome::IsRunningInMash()
BUG=606368
TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash.
Committed: https://crrev.com/92c518d4ecafd8a81a8e4aa4606d93bdb0eb53df
Cr-Commit-Position: refs/heads/master@{#400724}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed bug where mash chrome crashes accessing ash::Shell with context menus #
Total comments: 2
Patch Set 3 : Early return LEAVE_MENU_OPEN instead of CLOSE_MENU #Messages
Total messages: 23 (9 generated)
staraz@google.com changed reviewers: + msw@chromium.org, pkasting@chromium.org, sky@chromium.org
Please review my change
Thanks for posting this. A few notes: * Please write a CL description that explains what the bug was and what the fix is. Start with a single line of overall description, then go into more detail. * When listing multiple reviewers, say what you want each to review. Normally, reviewers don't need to overlap. In a case like this, you probably want just one reviewer. (And it probably shouldn't be me, despite my comments here, since I don't actually know anything about mash.) https://codereview.chromium.org/2073783003/diff/1/chrome/browser/ui/views/chr... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2073783003/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:274: if (chrome::IsRunningInMash()) { Nit: {} unnecessary https://codereview.chromium.org/2073783003/diff/1/chrome/browser/ui/views/chr... chrome/browser/ui/views/chrome_views_delegate.cc:275: return views::ViewsDelegate::ProcessMenuAcceleratorResult::CLOSE_MENU; Why is it correct to unconditionally close the menu, when the code below seems to check and do this conditionally?
Description was changed from ========== fixed issue 606368 BUG=606368 TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash. ========== to ========== fixed issue 606368: early return from ProcessAcceleratorWhileMenuShowing() if chrome::IsRunningInMash() BUG=606368 TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash. ==========
On 2016/06/16 17:02:39, Peter Kasting wrote: > Thanks for posting this. A few notes: > > * Please write a CL description that explains what the bug was and what the fix > is. Start with a single line of overall description, then go into more detail. > > * When listing multiple reviewers, say what you want each to review. Normally, > reviewers don't need to overlap. In a case like this, you probably want just > one reviewer. (And it probably shouldn't be me, despite my comments here, since > I don't actually know anything about mash.) > > https://codereview.chromium.org/2073783003/diff/1/chrome/browser/ui/views/chr... > File chrome/browser/ui/views/chrome_views_delegate.cc (right): > > https://codereview.chromium.org/2073783003/diff/1/chrome/browser/ui/views/chr... > chrome/browser/ui/views/chrome_views_delegate.cc:274: if > (chrome::IsRunningInMash()) { > Nit: {} unnecessary > > https://codereview.chromium.org/2073783003/diff/1/chrome/browser/ui/views/chr... > chrome/browser/ui/views/chrome_views_delegate.cc:275: return > views::ViewsDelegate::ProcessMenuAcceleratorResult::CLOSE_MENU; > Why is it correct to unconditionally close the menu, when the code below seems > to check and do this conditionally? Thank you for the comments! I have updated the title and description. I chose you as one of the reviewers because you are the owner of the file. I will ask my lead if he knows someone who could review it.
staraz@google.com changed reviewers: - msw@chromium.org, pkasting@chromium.org, sky@chromium.org
staraz@google.com changed reviewers: + msw@chromium.org, sadrul@chromium.org
Please review my change
I have updated the patch according to the comments. Please have another look.
https://codereview.chromium.org/2073783003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2073783003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:276: return views::ViewsDelegate::ProcessMenuAcceleratorResult::CLOSE_MENU; I think we should do the default here, i.e. return LEAVE_MENU_OPEN from here.
I have updated the patch to change the early return to LEAVE_MENU_OPEN https://codereview.chromium.org/2073783003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/chrome_views_delegate.cc (right): https://codereview.chromium.org/2073783003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/chrome_views_delegate.cc:276: return views::ViewsDelegate::ProcessMenuAcceleratorResult::CLOSE_MENU; On 2016/06/16 17:58:40, sadrul wrote: > I think we should do the default here, i.e. return LEAVE_MENU_OPEN from here. Acknowledged.
lgtm
The CQ bit was checked by staraz@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073783003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by staraz@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073783003/40001
Message was sent while issue was closed.
Description was changed from ========== fixed issue 606368: early return from ProcessAcceleratorWhileMenuShowing() if chrome::IsRunningInMash() BUG=606368 TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash. ========== to ========== fixed issue 606368: early return from ProcessAcceleratorWhileMenuShowing() if chrome::IsRunningInMash() BUG=606368 TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== fixed issue 606368: early return from ProcessAcceleratorWhileMenuShowing() if chrome::IsRunningInMash() BUG=606368 TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash. ========== to ========== fixed issue 606368: early return from ProcessAcceleratorWhileMenuShowing() if chrome::IsRunningInMash() BUG=606368 TEST=Start Chrome with mash. Right click on a webpage to open the context menu. Trigger a keyboard event by pressing the Ctrl key. Chrome should not crash. Committed: https://crrev.com/92c518d4ecafd8a81a8e4aa4606d93bdb0eb53df Cr-Commit-Position: refs/heads/master@{#400724} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/92c518d4ecafd8a81a8e4aa4606d93bdb0eb53df Cr-Commit-Position: refs/heads/master@{#400724} |
