Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(36)

Issue 11143031: Attempt at fixing crash in balloon code. The current code does a (Closed)

Created:
8 years, 2 months ago by sky
Modified:
8 years, 2 months ago
Reviewers:
stevenjb
CC:
chromium-reviews, tfarina
Visibility:
Public.

Description

Attempt at fixing crash in balloon code. The current code does a Widget::Close() and posts a delayed task to cleanup (delete this) after that. That means there is a window of time between when Close() completes and the task is run where the widget has been destroyed. I'm moving cleanup to DeleteDelegate() so there is no window. This change would also cover the case of the NativeWindow being deleted out from under the balloon. BUG=154173 TEST=none R=ben@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=162146

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -23 lines) Patch
M chrome/browser/notifications/balloon.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/notifications/balloon_view_views.h View 6 chunks +5 lines, -8 lines 0 comments Download
M chrome/browser/ui/views/notifications/balloon_view_views.cc View 4 chunks +10 lines, -14 lines 3 comments Download

Messages

Total messages: 4 (0 generated)
sky
8 years, 2 months ago (2012-10-16 04:28:57 UTC) #1
stevenjb
lgtm http://codereview.chromium.org/11143031/diff/1/chrome/browser/ui/views/notifications/balloon_view_views.cc File chrome/browser/ui/views/notifications/balloon_view_views.cc (right): http://codereview.chromium.org/11143031/diff/1/chrome/browser/ui/views/notifications/balloon_view_views.cc#newcode126 chrome/browser/ui/views/notifications/balloon_view_views.cc:126: // DeleteDelegate() and we'll cleanup. nit: it will, ...
8 years, 2 months ago (2012-10-16 16:43:41 UTC) #2
sky
http://codereview.chromium.org/11143031/diff/1/chrome/browser/ui/views/notifications/balloon_view_views.cc File chrome/browser/ui/views/notifications/balloon_view_views.cc (right): http://codereview.chromium.org/11143031/diff/1/chrome/browser/ui/views/notifications/balloon_view_views.cc#newcode126 chrome/browser/ui/views/notifications/balloon_view_views.cc:126: // DeleteDelegate() and we'll cleanup. On 2012/10/16 16:43:41, stevenjb ...
8 years, 2 months ago (2012-10-16 16:45:06 UTC) #3
stevenjb
8 years, 2 months ago (2012-10-16 16:59:53 UTC) #4
http://codereview.chromium.org/11143031/diff/1/chrome/browser/ui/views/notifi...
File chrome/browser/ui/views/notifications/balloon_view_views.cc (right):

http://codereview.chromium.org/11143031/diff/1/chrome/browser/ui/views/notifi...
chrome/browser/ui/views/notifications/balloon_view_views.cc:126: //
DeleteDelegate() and we'll cleanup.
On 2012/10/16 16:45:06, sky wrote:
> On 2012/10/16 16:43:41, stevenjb (chromium) wrote:
> > nit: it will, we will
> 
> What's wrong with contractions?
Strunk & White says don't use them :) But they can be challenging for non native
speakers, especially less common ones. Just a nit :) Feel free to disregard.

Powered by Google App Engine
This is Rietveld 408576698