Re-land: Don't keep recreating AXMenuListPopup

Source/modules/accessibility/AXMenuListPopup.cpp

 /*
  * Copyright (C) 2010 Apple 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:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 53 matching lines @@
 {
     return accessibilityIsIgnoredByDefault();
 }
 
 AXMenuListOption* AXMenuListPopup::menuListOptionAXObject(HTMLElement* element) const
 {
     ASSERT(element);
     if (!isHTMLOptionElement(*element))
         return 0;
 
-    AXObject* object = axObjectCache()->getOrCreate(MenuListOptionRole);
+    AXObject* object = axObjectCache()->getOrCreate(element);
+    if (!object)
+        return 0;
+
     ASSERT_WITH_SECURITY_IMPLICATION(object->isMenuListOption());
-
-    AXMenuListOption* option = toAXMenuListOption(object);
-    option->setElement(element);
-
-    return option;
+    return toAXMenuListOption(object);
 }
 
 int AXMenuListPopup::getSelectedIndex() const
 {
+    if (!m_parent)
+        return -1;
+
     Node* selectNode = m_parent->node();
     if (!selectNode)
         return -1;
 
     HTMLSelectElement* htmlSelectElement = toHTMLSelectElement(selectNode);
     return htmlSelectElement->selectedIndex();
 }
 
 bool AXMenuListPopup::press() const
 {
     if (!m_parent)
         return false;
 
     m_parent->press();
     return true;
 }
 
 void AXMenuListPopup::addChildren()
 {
     if (!m_parent)
         return;
 
     Node* selectNode = m_parent->node();
     if (!selectNode)
         return;
 
     HTMLSelectElement* htmlSelectElement = toHTMLSelectElement(selectNode);
     m_haveChildren = true;
 
-    m_activeIndex = getSelectedIndex();
+    if (m_activeIndex == -1)
+        m_activeIndex = getSelectedIndex();
+
     const WillBeHeapVector<RawPtrWillBeMember<HTMLElement>>& listItems = htmlSelectElement->listItems();
     unsigned length = listItems.size();
     for (unsigned i = 0; i < length; i++) {
         AXMenuListOption* option = menuListOptionAXObject(listItems[i]);
         if (option) {
             option->setParent(this);
             m_children.append(option);
         }
     }
 }
 
-void AXMenuListPopup::childrenChanged()
+void AXMenuListPopup::updateChildrenIfNecessary()

[je_julie(Not used), 2015/04/06 08:42:50]

From previous code, I can see childrenChanged() is called when children is
updated.
When I search 'updateChildrenIfNecessary()', it seems to be called when
'AXObject::children()' is called.
I'm not sure but do we need to clear and add children when AXObject::children()
is called?

[dmazzoni, 2015/04/06 15:42:12]

Hmmm, good point.

Looking again at AXLayoutObject::updateChildrenIfNecessary, it already does the
logic we need - we can just rely on it to call clearChildren only when necessary
and addChildren will be called automatically.

I deleted this override, the new test still passes.

 {
-    AXObjectCacheImpl* cache = axObjectCache();
-    if (!cache)
-        return;
-    for (size_t i = m_children.size(); i > 0 ; --i) {
-        AXObject* child = m_children[i - 1].get();
-        // FIXME: How could children end up in here that have no actionElement(), the check
-        // in menuListOptionAXObject would seem to prevent that.
-        if (child->actionElement()) {
-            child->detachFromParent();
-            cache->remove(child->axObjectID());
-        }
-    }
-
-    m_children.clear();
-    m_haveChildren = false;
+    clearChildren();
+    addChildren();
 }
 
 void AXMenuListPopup::didUpdateActiveOption(int optionIndex)
 {
     // We defer creation of the children until updating the active option so that we don't
     // create AXObjects for <option> elements while they're in the middle of removal.
     if (!m_haveChildren)
         addChildren();
 
-    ASSERT_ARG(optionIndex, optionIndex >= 0);
-    ASSERT_ARG(optionIndex, optionIndex < static_cast<int>(m_children.size()));
-
     AXObjectCacheImpl* cache = axObjectCache();
     if (m_activeIndex != optionIndex && m_activeIndex >= 0 && m_activeIndex < static_cast<int>(m_children.size())) {
         RefPtr<AXObject> previousChild = m_children[m_activeIndex].get();
         cache->postNotification(previousChild.get(), AXObjectCacheImpl::AXMenuListItemUnselected);
     }
 
-    RefPtr<AXObject> child = m_children[optionIndex].get();
-    cache->postNotification(child.get(), AXObjectCacheImpl::AXFocusedUIElementChanged);
-    cache->postNotification(child.get(), AXObjectCacheImpl::AXMenuListItemSelected);
+    if (optionIndex >= 0 && optionIndex < static_cast<int>(m_children.size())) {
+        RefPtr<AXObject> child = m_children[optionIndex].get();
+        cache->postNotification(child.get(), AXObjectCacheImpl::AXFocusedUIElementChanged);
+        cache->postNotification(child.get(), AXObjectCacheImpl::AXMenuListItemSelected);
+    }
+
     m_activeIndex = optionIndex;
 }
 
 void AXMenuListPopup::didHide()
 {
     AXObjectCacheImpl* cache = axObjectCache();
     cache->postNotification(this, AXObjectCacheImpl::AXHide);
     if (activeChild())
         cache->postNotification(activeChild(), AXObjectCacheImpl::AXMenuListItemUnselected);
 }
@@ skipping 14 matching lines @@
 
 AXObject* AXMenuListPopup::activeChild()
 {
     if (m_activeIndex < 0 || m_activeIndex >= static_cast<int>(children().size()))
         return nullptr;
 
     return m_children[m_activeIndex].get();
 }
 
 } // namespace blink