Fix MenuButton for the case where multiple listeners are defined.

Fix MenuButton for the case where multiple listeners are defined.

This is a minimal fix suitable for merging to M38. The underlying problem
is that we create a new MenuButton instance (with its own click handler)
for every connection. I recently changed the behaviour of MenuButton so
that it toggles the 'active' class instead of adding it so that clicking
on the button while a menu is open will dismiss it. However, this breaks
if there an even number of MenuButton instances for a single DOM element.

The correct fix is to make the tool-bar buttons owned by the tool-bar,
which is a singleton. However, this minimal fix, which restores the M37
behaviour, is a better option for merging to M38.

BUG=415410
Committed: https://crrev.com/2405b3063aa466a5133cac621962068088381087
Cr-Commit-Position: refs/heads/master@{#295759}

[Jamie, 2014-09-18 22:31:03]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2014-09-18 22:31:03]

ptal

[kelvinp, 2014-09-18 23:28:56]

I think the root cause is that we don't clean up the state of element upon
destruction of the menu button.  

For now, what about simply removing the active class upon construction of the
button?  In this way, even in the future when we need to initialize the menu
button twice, it will still work.

[Jamie, 2014-09-18 23:50:33]

On 2014/09/18 23:28:56, kelvinp wrote:
> I think the root cause is that we don't clean up the state of element upon
> destruction of the menu button.  
> 
> For now, what about simply removing the active class upon construction of the
> button?  In this way, even in the future when we need to initialize the menu
> button twice, it will still work.

The problem isn't the state of the element in the ctor, it's the fact that we
end up with two events handlers installed. I don't think we'll ever want two
MenuButton instances attached to the same element. We can perhaps guard against
that via an assert in the ctor in the future, but for this CL I just want to
make sure it doesn't regress the previous behaviour and is suitable for merging.
I plan to follow-up with a CL to fix the problem more cleanly.

[kelvinp, 2014-09-19 00:33:22]

lgtm

[Jamie, 2014-09-19 00:46:37]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-09-19 00:48:44]

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

[commit-bot: I haz the power, 2014-09-19 01:47:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 01:47:46]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jamie, 2014-09-19 16:08:37]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-09-19 16:09:03]

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

[commit-bot: I haz the power, 2014-09-19 18:03:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 18:03:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jamie, 2014-09-19 18:39:33]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-09-19 18:40:54]

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

[commit-bot: I haz the power, 2014-09-19 19:22:11]

Committed patchset #2 (id:20001) as deeef224cc0522b9178e75ab5199685d2ed14994

[commit-bot: I haz the power, 2014-09-19 19:22:57]

Patchset 2 (id:??) landed as
https://crrev.com/2405b3063aa466a5133cac621962068088381087
Cr-Commit-Position: refs/heads/master@{#295759}