Fixes crash by checking for NULL when showing popup. This can happen

Fixes crash by checking for NULL when showing popup. This can happen
because ImmGetDefaultIMEWnd may trigger a focus lost, thereby closing
the popup so that when we get to Show the popup is NULL.

BUG=92497
TEST=none
R=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99773

[Ben Goodger (Google), 2011-09-06 15:43:49]

LGTM

On Tue, Sep 6, 2011 at 8:38 AM, <sky@chromium.org> wrote:

> Reviewers: Ben Goodger (Google),
>
> Description:
> Fixes crash by checking for NULL when showing popup. This can happen
> because ImmGetDefaultIMEWnd may trigger a focus lost, thereby closing
> the popup so that when we get to Show the popup is NULL.
>
> BUG=92497
> TEST=none
> R=ben@chromium.org
>
>
> Please review this at
http://codereview.chromium.**org/7833045/<http://codereview.chromium.org/7833...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/ui/views/**autocomplete/autocomplete_**
> popup_contents_view.cc
>
>
> Index: chrome/browser/ui/views/**autocomplete/autocomplete_**
> popup_contents_view.cc
> diff --git
a/chrome/browser/ui/views/**autocomplete/autocomplete_**popup_contents_view.cc
> b/chrome/browser/ui/views/**autocomplete/autocomplete_**
> popup_contents_view.cc
> index 10cf6882f4c57f48b7df489ec54131**2001e3b863..**
> bf104f26523436520b0a121abe16b4**1ed077b56d 100644
> --- a/chrome/browser/ui/views/**autocomplete/autocomplete_**
> popup_contents_view.cc
> +++ b/chrome/browser/ui/views/**autocomplete/autocomplete_**
> popup_contents_view.cc
> @@ -364,6 +364,13 @@ void
AutocompletePopupContentsView:**:UpdatePopupAppearance()
> {
>     popup_->SetContentsView(this);
>     popup_->MoveAbove(
>         GetRelativeWindowForPopup(**omnibox_view_->GetNativeView()**));
> +    if (!popup_.get()) {
> +      // For some IMEs GetRelativeWindowForPopup triggers the omnibox to
> lose
> +      // focus, thereby closing (and destroying) the popup.
> +      // TODO: this won't be needed once we close the omnibox on input
> window
> +      // showing.
> +      return;
> +    }
>     popup_->Show();
>   } else {
>     // Animate the popup shrinking, but don't animate growing larger since
> that
>
>
>

[Peter Kasting, 2011-09-12 18:36:52]

Did you investigate or file a bug for the stuff we chatted about before I left
on vacation?  Namely, that we'd like to detect this case and somehow avoid
losing focus or closing the popup, if feasible?

http://codereview.chromium.org/7833045/diff/1/chrome/browser/ui/views/autocom...
File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/7833045/diff/1/chrome/browser/ui/views/autocom...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:370: //
TODO: this won't be needed once we close the omnibox on input window
? We explicitly wanted to avoid closing the omnibox when the input window was
showing, hence the current code that tries to avoid overlapping the two.

[sky, 2011-09-12 20:19:29]

Yes. There was a thread with the guys in Tokyo (sorry I didn't cc you,
but I did forward the last email in the thread). They are
investigating closing the popup when the IME window is active. Given
that, this workaround seemed fine for the time being.

  -Scott

On Mon, Sep 12, 2011 at 11:36 AM,  <pkasting@chromium.org> wrote:
> Did you investigate or file a bug for the stuff we chatted about before I
> left
> on vacation?  Namely, that we'd like to detect this case and somehow avoid
> losing focus or closing the popup, if feasible?
>
>
>
http://codereview.chromium.org/7833045/diff/1/chrome/browser/ui/views/autocom...
> File
> chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
> (right):
>
>
http://codereview.chromium.org/7833045/diff/1/chrome/browser/ui/views/autocom...
> chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:370:
> // TODO: this won't be needed once we close the omnibox on input window
> ? We explicitly wanted to avoid closing the omnibox when the input
> window was showing, hence the current code that tries to avoid
> overlapping the two.
>
> http://codereview.chromium.org/7833045/
>