Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9535020:
Fix DCHECK in BrowserActionsContainer::OnWidgetClosing (Closed)
|
Created:
8 years, 9 months ago by stevenjb Modified:
8 years, 9 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix DCHECK in BrowserActionsContainer::OnWidgetClosing
BUG=116270
TEST=See issue
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125006
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix spelling nit #
Total comments: 2
Patch Set 3 : Remove observer in BrowserActionsContainer::HidePopup() #
Total comments: 2
Patch Set 4 : Fix comments #
Total comments: 2
Patch Set 5 : Restore DCHECK #Patch Set 6 : Rebase #Messages
Total messages: 18 (0 generated)
LGTM with spelling nit. http://codereview.chromium.org/9535020/diff/1/chrome/browser/ui/views/browser... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/1/chrome/browser/ui/views/browser... chrome/browser/ui/views/browser_actions_container.cc:844: // Set the popup button to not pused here, since we might change popup_ spelling nit: pushed
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9535020/4001
Presubmit check for 9535020-4001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/ui/views/browser_actions_container.cc
+sky for OWNERS
http://codereview.chromium.org/9535020/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:842: popup_->GetWidget()->Close(); I think we should remove the observer at line 841 so that we don't have the situation of a delayed OnWidgetclosing causing problems.
http://codereview.chromium.org/9535020/diff/4001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/4001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:842: popup_->GetWidget()->Close(); On 2012/03/01 21:06:01, sky wrote: > I think we should remove the observer at line 841 so that we don't have the > situation of a delayed OnWidgetclosing causing problems. Agreed. Added that and set popup_ to NULL here + cleaned up comments.
http://codereview.chromium.org/9535020/diff/9001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/9001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:841: // Remove this as an observer and clear popup_ and popup_button_ here, nit: add vertical bars around identifiers in comments.
http://codereview.chromium.org/9535020/diff/9001/chrome/browser/ui/views/brow... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/9001/chrome/browser/ui/views/brow... chrome/browser/ui/views/browser_actions_container.cc:841: // Remove this as an observer and clear popup_ and popup_button_ here, On 2012/03/02 20:39:35, msw wrote: > nit: add vertical bars around identifiers in comments. Done.
http://codereview.chromium.org/9535020/diff/12001/chrome/browser/ui/views/bro... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/12001/chrome/browser/ui/views/bro... chrome/browser/ui/views/browser_actions_container.cc:818: // If a new popup has been opened before this gets called, |popup_| will Can't this go back to a DCHECK now?
http://codereview.chromium.org/9535020/diff/12001/chrome/browser/ui/views/bro... File chrome/browser/ui/views/browser_actions_container.cc (right): http://codereview.chromium.org/9535020/diff/12001/chrome/browser/ui/views/bro... chrome/browser/ui/views/browser_actions_container.cc:818: // If a new popup has been opened before this gets called, |popup_| will On 2012/03/02 20:53:50, sky wrote: > Can't this go back to a DCHECK now? Since closing a window is independent from calls to HidePopup(), it seems possible that OnWidgetClosing() might get called for window A after HidePopup() gets called for window B, in which case popup_ might be NULL. This code should be safe regardless. I'm not certain that a DCHECK would be.
On Fri, Mar 2, 2012 at 1:29 PM, <stevenjb@chromium.org> wrote: > > http://codereview.chromium.org/9535020/diff/12001/chrome/browser/ui/views/bro... > File chrome/browser/ui/views/browser_actions_container.cc (right): > > http://codereview.chromium.org/9535020/diff/12001/chrome/browser/ui/views/bro... > chrome/browser/ui/views/browser_actions_container.cc:818: // If a new > popup has been opened before this gets called, |popup_| will > On 2012/03/02 20:53:50, sky wrote: >> >> Can't this go back to a DCHECK now? > > > Since closing a window is independent from calls to HidePopup(), it > seems possible that OnWidgetClosing() might get called for window A > after HidePopup() gets called for window B, in which case popup_ might > be NULL. This code should be safe regardless. I'm not certain that a > DCHECK would be. But we're now removing the observer in HidePopup, so I don't see how we could ever get into a situation where OnWidgetClosing is passed a widget other than popup_. -Scott
I guess I just didn't want to assume that OnWidgetClosing(A) would always get called before OnBrowserActionExecuted(B) when B is opened while A is still open or is closing, however it appears that is always true at least in Aura, so I reverted that change.
Because of your change to remove the observer before we initiate the close this can never happen. LGTM On Fri, Mar 2, 2012 at 3:51 PM, <stevenjb@chromium.org> wrote: > I guess I just didn't want to assume that OnWidgetClosing(A) would always > get > called before OnBrowserActionExecuted(B) when B is opened while A is still > open > or is closing, however it appears that is always true at least in Aura, so I > reverted that change. > > > http://codereview.chromium.org/9535020/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9535020/10003
Try job failure for 9535020-10003 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9535020/20003
Change committed as 125006 |
