|
|
Created:
6 years, 9 months ago by Tommy Widenflycht Modified:
6 years, 8 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixing a lifetime issue for Speech Recognition Bubble
It seems that on a slow XP machine the view can be deleted before the
Impl. Fixed by a simple observer pattern.
BUG=352851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261737
Patch Set 1 #
Total comments: 6
Patch Set 2 : ixed comments #
Total comments: 4
Patch Set 3 : Fixed Comments #
Total comments: 2
Patch Set 4 : Refactored using WidgetObserver #Patch Set 5 : Fixed tests #Messages
Total messages: 19 (0 generated)
Tomi, can you do an initial sanity pass, please?
https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right): https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... chrome/browser/ui/views/speech_recognition_bubble_views.cc:53: void SetLifeTImeObserver( SetLifeTimeObserver (lowercase i) https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... chrome/browser/ui/views/speech_recognition_bubble_views.cc:55: life_time_observer_ = life_time_observer; fix indent https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... chrome/browser/ui/views/speech_recognition_bubble_views.cc:135: BubbleDelegateView::OnWidgetDestroyed(widget); fix indent
https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right): https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... chrome/browser/ui/views/speech_recognition_bubble_views.cc:53: void SetLifeTImeObserver( On 2014/03/26 17:01:43, tommi wrote: > SetLifeTimeObserver > (lowercase i) Done. https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... chrome/browser/ui/views/speech_recognition_bubble_views.cc:55: life_time_observer_ = life_time_observer; On 2014/03/26 17:01:43, tommi wrote: > fix indent Done. https://codereview.chromium.org/213153002/diff/1/chrome/browser/ui/views/spee... chrome/browser/ui/views/speech_recognition_bubble_views.cc:135: BubbleDelegateView::OnWidgetDestroyed(widget); On 2014/03/26 17:01:43, tommi wrote: > fix indent Done.
lgtm https://codereview.chromium.org/213153002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right): https://codereview.chromium.org/213153002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/speech_recognition_bubble_views.cc:134: life_time_observer_ = life_time_observer; nit: First DCHECK(!life_time_observer_)? or if life_time_observer can be NULL, DCHECK(life_time_observer_ == life_time_observer || (!life_time_observer_ && life_time_observer) || (life_time_observer_ && !life_time_observer)); https://codereview.chromium.org/213153002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/speech_recognition_bubble_views.cc:139: if (life_time_observer_) { nit: no {}
msw@ & sky@: Need OWNERS approval https://codereview.chromium.org/213153002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right): https://codereview.chromium.org/213153002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/speech_recognition_bubble_views.cc:134: life_time_observer_ = life_time_observer; On 2014/03/27 14:44:03, tommi wrote: > nit: First DCHECK(!life_time_observer_)? > or if life_time_observer can be NULL, > DCHECK(life_time_observer_ == life_time_observer || > (!life_time_observer_ && life_time_observer) || > (life_time_observer_ && !life_time_observer)); Done. https://codereview.chromium.org/213153002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/speech_recognition_bubble_views.cc:139: if (life_time_observer_) { On 2014/03/27 14:44:03, tommi wrote: > nit: no {} Done.
LGTM
https://codereview.chromium.org/213153002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right): https://codereview.chromium.org/213153002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/speech_recognition_bubble_views.cc:34: class SpeechRecognitionBubbleViewLifeTimeObserver { Use the pre-existing WidgetObserver, which already has an OnWidgetDestroyed callback, and wouldn't necessitate any changes to the SpeechRecognitionBubbleView class.
https://codereview.chromium.org/213153002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/speech_recognition_bubble_views.cc (right): https://codereview.chromium.org/213153002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/speech_recognition_bubble_views.cc:34: class SpeechRecognitionBubbleViewLifeTimeObserver { On 2014/03/31 16:53:30, msw wrote: > Use the pre-existing WidgetObserver, which already has an OnWidgetDestroyed > callback, and wouldn't necessitate any changes to the > SpeechRecognitionBubbleView class. Done.
lgtm
The CQ bit was checked by tommyw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommyw@chromium.org/213153002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by tommyw@chromium.org
The CQ bit was unchecked by tommyw@chromium.org
The CQ bit was checked by tommyw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommyw@chromium.org/213153002/80001
Message was sent while issue was closed.
Change committed as 261737 |