Add unit-tests for MenuButton and simplify implementation.

Add unit-tests for MenuButton and simplify implementation.

The old method of using a timer to set the closeHandler was messy and harder
to unit-test. The new method uses stopPropagation, which is cleaner and makes
the unit-tests simpler.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284295

[Jamie, 2014-07-18 01:37:19]

ptal

https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/menu_but...
File remoting/webapp/menu_button.css (left):

https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/menu_but...
remoting/webapp/menu_button.css:11: visibility: hidden;
visibility: hidden doesn't allow for verification using offsetWidth/Height. I
could use getComputedStyle instead; any preference?

[kelvinp, 2014-07-18 18:47:39]

lgtm

https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/menu_but...
File remoting/webapp/menu_button.css (left):

https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/menu_but...
remoting/webapp/menu_button.css:11: visibility: hidden;
On 2014/07/18 01:37:18, Jamie wrote:
> visibility: hidden doesn't allow for verification using offsetWidth/Height. I
> could use getComputedStyle instead; any preference?

I think either way is fine.  Personally, I prefer getComputedStyle and check for
the visibility or display or the element instead of checking the width and
height.

[Jamie, 2014-07-18 19:46:12]

https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/menu_but...
File remoting/webapp/menu_button.css (left):

https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/menu_but...
remoting/webapp/menu_button.css:11: visibility: hidden;
On 2014/07/18 18:47:39, kelvinp wrote:
> On 2014/07/18 01:37:18, Jamie wrote:
> > visibility: hidden doesn't allow for verification using offsetWidth/Height.
I
> > could use getComputedStyle instead; any preference?
> 
> I think either way is fine.  Personally, I prefer getComputedStyle and check
for
> the visibility or display or the element instead of checking the width and
> height.

As discussed in person, since showing or hiding the element requires a
re-layout, it would need an asynchronous browser test to do it that way, and
might be more flaky. I'm going to stick with this approach.

[Jamie, 2014-07-18 19:46:15]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-07-18 19:47:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/401623004/20001

[commit-bot: I haz the power, 2014-07-18 19:47:54]

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

[commit-bot: I haz the power, 2014-07-18 19:47:55]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[garykac, 2014-07-18 19:52:40]

lgtm

[Jamie, 2014-07-18 19:53:17]

The CQ bit was unchecked by jamiewalch@chromium.org

[Jamie, 2014-07-18 19:53:20]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-07-18 19:55:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/401623004/20001

[commit-bot: I haz the power, 2014-07-18 20:14:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30506)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/34917)

[commit-bot: I haz the power, 2014-07-18 20:16:51]

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

[commit-bot: I haz the power, 2014-07-18 20:16:52]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30515)

[Jamie, 2014-07-18 20:34:43]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-07-18 20:36:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/401623004/40001

[commit-bot: I haz the power, 2014-07-18 23:53:38]

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

[commit-bot: I haz the power, 2014-07-18 23:53:39]

Failed to apply patch for remoting/remoting_webapp_files.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file remoting/remoting_webapp_files.gypi
  Hunk #1 FAILED at 136.
  1 out of 1 hunk FAILED -- saving rejects to file
remoting/remoting_webapp_files.gypi.rej

Patch:       remoting/remoting_webapp_files.gypi
Index: remoting/remoting_webapp_files.gypi
diff --git a/remoting/remoting_webapp_files.gypi
b/remoting/remoting_webapp_files.gypi
index
5bb645dada9908e08944fbed3fca2427c3d4364c..81eee32f5aad54e251e79f24eb119490b4a87e39
100644
--- a/remoting/remoting_webapp_files.gypi
+++ b/remoting/remoting_webapp_files.gypi
@@ -136,6 +136,10 @@
     # The unit test cases for the webapp
     'remoting_webapp_unittest_cases': [
       'webapp/unittests/test_eventSource.js',
+      'webapp/unittests/menu_button_unittest.js',
+    ],
+    'remoting_webapp_unittest_additional_files': [
+      'webapp/menu_button.css',
     ],
     'remoting_webapp_unittest_template_main':
       'webapp/html/template_unittest.html',

[Jamie, 2014-07-19 00:06:39]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-07-19 00:07:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/401623004/60001

[commit-bot: I haz the power, 2014-07-19 02:59:59]

Change committed as 284295