Re-land: Don't keep recreating AXMenuListPopup

Re-land: Don't keep recreating AXMenuListPopup

Every time an AXMenuList (corresponding to a
<select> element) needed to update its children,
we were recreating the AXMenuListPopup and
all of the AXMenuListOptions each time, which is
wasteful. This change avoids deleting the popup
and stores the options in AXObjectCache by
element, rather than by ID, so they can be reused
when the options are updated.

BUG=323462
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193517

[dmazzoni, 2015-04-06 05:21:31]

dmazzoni@chromium.org changed reviewers:
+ je_julie.kim@samsung.com

[je_julie(Not used), 2015-04-06 08:42:51]

Please take a look at the comment.

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListOption.cpp (right):

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListOption.cpp:29: #include
"core/html/HTMLOptionElement.h"
You can remove "HTMLOptionElement.h" because you added it in AXMenuListOption.h.

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListPopup.cpp (right):

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListPopup.cpp:130: void
AXMenuListPopup::updateChildrenIfNecessary()
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?

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:213: if (!parent ||
!isHTMLSelectElement(parent))
You can remove !parent because isHTMLSelectElement(*) includes it.

[dmazzoni, 2015-04-06 15:42:13]

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListOption.cpp (right):

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListOption.cpp:29: #include
"core/html/HTMLOptionElement.h"
On 2015/04/06 08:42:50, je_julie wrote:
> You can remove "HTMLOptionElement.h" because you added it in
AXMenuListOption.h.

Done.

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXMenuListPopup.cpp (right):

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXMenuListPopup.cpp:130: void
AXMenuListPopup::updateChildrenIfNecessary()
On 2015/04/06 08:42:50, je_julie wrote:
> 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?

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.

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/1059503002/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXObjectCacheImpl.cpp:213: if (!parent ||
!isHTMLSelectElement(parent))
On 2015/04/06 08:42:51, je_julie wrote:
> You can remove !parent because isHTMLSelectElement(*) includes it.

Done.

[je_julie(Not used), 2015-04-07 02:39:12]

On 2015/04/06 15:42:13, dmazzoni wrote:
> Source/modules/accessibility/AXMenuListPopup.cpp:130: void
> AXMenuListPopup::updateChildrenIfNecessary()
> On 2015/04/06 08:42:50, je_julie wrote:
> > 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?
> 
> 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.

Thanks, Dominic.
I can see that AXLayoutObject::updateChildrenIfNecessary has
needsToUpdateChildren() condition.
Doesn't void AXMenuListPopup::updateChildrenIfNecessary() also need some
condition?

[dmazzoni, 2015-04-07 06:28:58]

On 2015/04/07 02:39:12, je_julie wrote:
> Thanks, Dominic.
> I can see that AXLayoutObject::updateChildrenIfNecessary has
> needsToUpdateChildren() condition.
> Doesn't void AXMenuListPopup::updateChildrenIfNecessary() also need some
> condition?

Good point! Since we're just relying on whether the parent element (AXMenuList)
needs to have its children updated or not, we can check that and only update it
if needed.

I also made a slight simplification and deleted AXMenuList::childrenChanged,
it's not necessary to override it anymore because the default implementation
will call setNeedsToUpdateChildren(), which is exactly what we need.

Note that the case of adding an option to a menu list is already covered by an
existing test. I confirmed that deliberately breaking the logic in this
changelist makes that test fail.

[je_julie(Not used), 2015-04-07 06:59:39]

On 2015/04/07 06:28:58, dmazzoni wrote:
> On 2015/04/07 02:39:12, je_julie wrote:
> > Thanks, Dominic.
> > I can see that AXLayoutObject::updateChildrenIfNecessary has
> > needsToUpdateChildren() condition.
> > Doesn't void AXMenuListPopup::updateChildrenIfNecessary() also need some
> > condition?
> 
> Good point! Since we're just relying on whether the parent element
(AXMenuList)
> needs to have its children updated or not, we can check that and only update
it
> if needed.
> 
> I also made a slight simplification and deleted AXMenuList::childrenChanged,
> it's not necessary to override it anymore because the default implementation
> will call setNeedsToUpdateChildren(), which is exactly what we need.
> 
> Note that the case of adding an option to a menu list is already covered by an
> existing test. I confirmed that deliberately breaking the logic in this
> changelist makes that test fail.

Great! lgtm.

[dmazzoni, 2015-04-07 07:14:30]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-04-07 07:14:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059503002/140001

[commit-bot: I haz the power, 2015-04-07 07:36:58]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193251

[Stephen Chennney, 2015-04-07 17:54:50]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1061063004/ by schenney@chromium.org.

The reason for reverting is: This appears to be blocking the Blink roll due to
the failure of DumpAccessibilityTreeTest.AccessibilityModalDialogClosed

See, for example, this attempted roll:
https://codereview.chromium.org/1068753002/.

[dmazzoni, 2015-04-09 20:50:15]

This change got reverted because it broke <optgroup>. I fixed it and added a new
test; please take a look again, thanks!

[je_julie(Not used), 2015-04-10 04:06:58]

On 2015/04/09 20:50:15, dmazzoni wrote:
> This change got reverted because it broke <optgroup>. I fixed it and added a
new
> test; please take a look again, thanks!

lgtm!

[dmazzoni, 2015-04-10 06:28:37]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-04-10 06:29:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059503002/160001

[commit-bot: I haz the power, 2015-04-10 06:32:44]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193517