Fix PopupMenuChromium to hide itself before calling valueChanged. This bette...

third_party/WebKit/WebCore/platform/chromium/PopupMenuChromium.cpp

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  * 
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  * 
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 55 matching lines @@
 
 static const int kMaxVisibleRows = 20;
 static const int kMaxHeight = 500;
 static const int kBorderSize = 1;
 static const TimeStamp kTypeAheadTimeoutMs = 1000;
 
 // This class uses WebCore code to paint and handle events for a drop-down list
 // box ("combobox" on Windows).
 class PopupListBox : public FramelessScrollView, public RefCounted<PopupListBox> {
 public:
+    static PassRefPtr<PopupListBox> create(PopupMenuClient* client)
+    {
+        return adoptRef(new PopupListBox(client));
+    }
+
     // FramelessScrollView
     virtual void paint(GraphicsContext*, const IntRect&);
     virtual bool handleMouseDownEvent(const PlatformMouseEvent&);
     virtual bool handleMouseMoveEvent(const PlatformMouseEvent&);
     virtual bool handleMouseReleaseEvent(const PlatformMouseEvent&);
     virtual bool handleWheelEvent(const PlatformWheelEvent&);
     virtual bool handleKeyEvent(const PlatformKeyboardEvent&);
 
     // ScrollView
     virtual HostWindow* hostWindow() const;
@@ skipping 230 matching lines @@
 // PopupContainer implementation
 
 // static
 PassRefPtr<PopupContainer> PopupContainer::create(PopupMenuClient* client,
                                                   bool focusOnShow)
 {
     return adoptRef(new PopupContainer(client, focusOnShow));
 }
 
 PopupContainer::PopupContainer(PopupMenuClient* client, bool focusOnShow)
-    : m_listBox(new PopupListBox(client)),
-      m_focusOnShow(focusOnShow)
+    : m_listBox(PopupListBox::create(client))
+    , m_focusOnShow(focusOnShow)
 {
-    // FrameViews are created with a refcount of 1 so it needs releasing after we
-    // assign it to a RefPtr.
-    m_listBox->deref();
-
     setScrollbarModes(ScrollbarAlwaysOff, ScrollbarAlwaysOff);
 }
 
 PopupContainer::~PopupContainer()
 {
     if (m_listBox)
         removeChild(m_listBox.get());
 }
 
 void PopupContainer::showPopup(FrameView* view)
@@ skipping 11 matching lines @@
         // If the popup would extend past the bottom of the screen, open upwards
         // instead.
         FloatRect screen = screenRect(view);
         IntRect widgetRect = chromeClient->windowToScreen(frameRect());
         if (widgetRect.bottom() > static_cast<int>(screen.bottom()))
             widgetRect.move(0, -(widgetRect.height() + selectHeight));
 
         chromeClient->popupOpened(this, widgetRect, m_focusOnShow);
     }
 
-    // Must get called after we have a client and containingWindow.
-    addChild(m_listBox.get());
+    if (!m_listBox->parent())
+        addChild(m_listBox.get());
 
-    // Enable scrollbars after the listbox is inserted into the hierarchy, so
-    // it has a proper WidgetClient.
+    // Enable scrollbars after the listbox is inserted into the hierarchy,
+    // so it has a proper WidgetClient.
     m_listBox->setVerticalScrollbarMode(ScrollbarAuto);
 
     m_listBox->scrollToRevealSelection();
 
     invalidate();
 }
 
 void PopupContainer::hidePopup()
 {
-    invalidate();
-
-    m_listBox->disconnectClient();
-    removeChild(m_listBox.get());
-    m_listBox = 0;
-    
     if (client())
         client()->popupClosed(this);
 }
 
 void PopupContainer::layout()
 {
     m_listBox->layout();
 
     // Place the listbox within our border.
     m_listBox->move(kBorderSize, kBorderSize);
@@ skipping 455 matching lines @@
 
     return itemFont;
 }
 
 void PopupListBox::abandon()
 {
     RefPtr<PopupListBox> keepAlive(this);
 
     m_selectedIndex = m_originalIndex;
 
+    m_popupClient->hidePopup();
+
     if (m_willAcceptOnAbandon)
         m_popupClient->valueChanged(m_selectedIndex);
-
-    // valueChanged may have torn down the popup!
-    if (m_popupClient)
-        m_popupClient->hidePopup();
 }
 
 int PopupListBox::pointToRowIndex(const IntPoint& point)
 {
     int y = scrollY() + point.y();
 
     // FIXME: binary search if perf matters.
     for (int i = 0; i < numItems(); ++i) {
         if (y < m_items[i]->y)
             return i-1;
@@ skipping 11 matching lines @@
     ASSERT(index >= -1 && index < numItems());
     if (index == -1 && m_popupClient) {
       // Enter pressed with no selection, just close the popup.
       m_popupClient->hidePopup();
       return;
     }
 
     if (isSelectableItem(index)) {
         RefPtr<PopupListBox> keepAlive(this);
 
-        // Tell the <select> PopupMenuClient what index was selected, and hide ourself.
+        // Hide ourselves first since valueChanged may have numerous side-effects.
+        m_popupClient->hidePopup();
+
+        // Tell the <select> PopupMenuClient what index was selected.
         m_popupClient->valueChanged(index);
-
-        // valueChanged may have torn down the popup!
-        if (m_popupClient)
-            m_popupClient->hidePopup();
     }
 }
 
 void PopupListBox::selectIndex(int index)
 {
     ASSERT(index >= 0 && index < numItems());
 
     if (index != m_selectedIndex && isSelectableItem(index)) {
         invalidateRow(m_selectedIndex);
         m_selectedIndex = index;
@@ skipping 238 matching lines @@
 {
 }
 
 PopupMenu::~PopupMenu()
 {
     hide();
 }
 
 void PopupMenu::show(const IntRect& r, FrameView* v, int index) 
 {
-    p.popup = PopupContainer::create(client(), true);
+    if (!p.popup)
+        p.popup = PopupContainer::create(client(), true);
     p.popup->show(r, v, index);
 }
 
 void PopupMenu::hide()
 {
-    if (p.popup) {
+    if (p.popup)
         p.popup->hidePopup();
-        p.popup = 0;
-    }
 }
 
 void PopupMenu::updateFromElement()
 {
     p.popup->listBox()->updateFromElement();
 }
 
 bool PopupMenu::itemWritingDirectionIsNatural() 
 { 
     return false; 
 }
 
 } // namespace WebCore