Bug 23696 – CHROMIUM: select element doesn't show new value when focus is switched in onchange event

• Component: Platform
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

CHROMIUM: select element doesn't show new value when focus is switched in onchange event

Original bug report:
http://code.google.com/p/chromium/issues/detail?id=3514

It looks like we need to hide the PopupMenu before calling valueChanged.  Patch coming up.

[Darin Fisher (:fishd, Google)]

Created attachment 27258 [details]
test case

[Darin Fisher (:fishd, Google)]

Created attachment 27262 [details]
v1 patch

This already has R=ojan here:
http://codereview.chromium.org/19765

[Eric Seidel (no email)]

Comment on attachment 27262 [details]
v1 patch

     if (!m_listBox->parent()) {
 366         // Must get called after we have a client.
 367         addChild(m_listBox.get());
 368     }

Why?


Also:
 PopupMenu::~PopupMenu()
 {
     hide();
+    p.popup = 0;
 }

is p.popup a smartpointer?  If so, we should consider using .clear() instead.  If it's not, I expect we're leaking the PopupContainer.

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> (From update of attachment 27262 [details] [review])
>      if (!m_listBox->parent()) {
>  366         // Must get called after we have a client.
>  367         addChild(m_listBox.get());
>  368     }

Whoa, that is actually a very stale comment.  I removed it.


> Also:
>  PopupMenu::~PopupMenu()
>  {
>      hide();
> +    p.popup = 0;
>  }
> 
> is p.popup a smartpointer?  If so, we should consider using .clear() instead. 
> If it's not, I expect we're leaking the PopupContainer.

Good catch.  Nulling out this RefPtr is definitely extraneous.

[Darin Fisher (:fishd, Google)]

Created attachment 27264 [details]
v2 patch

[Eric Seidel (no email)]

Comment on attachment 27264 [details]
v2 patch

LGTM.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/40504