Remove the blue tool-bar for apps v2.

Remove the blue tool-bar for apps v2.

Its visual style is not really in keeping with the rest of the v2 app,
and the form-factor is not conducive to an easily extensible UI. For now,
a menu will suffice; if we need anything more complex, it can easily be
replaced with an options dialog.

This CL also simplifies the MenuButton class to use stopPropagation instead of timers to prevent the close handler being triggered by the same click that opens the menu.

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

[Jamie, 2014-07-17 17:04:03]

ptal

https://codereview.chromium.org/339613003/diff/60001/remoting/resources/remot...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/339613003/diff/60001/remoting/resources/remot...
remoting/resources/remoting_strings.grd:720: Send Ctrl-Alt-Del
The window frame has these strings as a top-level menu items so we need a verb.
The tool-bar has them under a "Send keys" menu, so the the verb is redundant,
but in the interests of sharing the strings, it doesn't do any harm.

https://codereview.chromium.org/339613003/diff/60001/remoting/webapp/feedback.js
File remoting/webapp/feedback.js (left):

https://codereview.chromium.org/339613003/diff/60001/remoting/webapp/feedback...
remoting/webapp/feedback.js:21: var menuButton = new
remoting.MenuButton(helpIcon);
Since these menu items are in a shared (rather than separate) menu in the window
frame, we don't want to create the MenuButton. There's also no need for this to
be a class, and it can possibly go away altogether once we don't have to support
pre-M35.

https://codereview.chromium.org/339613003/diff/60001/remoting/webapp/menu_but...
File remoting/webapp/menu_button.js (right):

https://codereview.chromium.org/339613003/diff/60001/remoting/webapp/menu_but...
remoting/webapp/menu_button.js:49: * @param {Element} item The menu item to
update.
Previously, this was only ever called with the result of getElementById, which
has type HTMLElement. Now, it is also called with the result of querySelector,
which has type Element. Unfortunately, that means we need the ugly DOMTokenList
cast, below.

[kelvinp, 2014-07-17 21:59:51]

Would it be a good opportunity to add some unit test for MenuButton.js?

[Jamie, 2014-07-17 22:24:47]

On 2014/07/17 21:59:51, kelvinp wrote:
> Would it be a good opportunity to add some unit test for MenuButton.js?

Good idea. I think I'll pull out the MenuButton changes into a separate CL,
since they're only tangentially related to this one, and I'll add unit tests
there.

[kelvinp, 2014-07-18 18:27:35]

lgtm

[garykac, 2014-07-22 22:41:45]

lgtm

[Jamie, 2014-07-23 16:57:00]

PTAL, as quite a lot has changed since the last review. Specifically:

* Most of the changes to MenuButton were factored out into a separate CL.
* I had to add an onHide callback to MenuButton to get the right behaviour for
the pop-up menu in full-screen mode.
* I'd forgotten about the "New connection" button in the apps v2 tool-bar, so I
added that functionality to OptionsMenu.

[kelvinp, 2014-07-23 18:49:08]

LGTM once my comments are addressed

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/unittes...
File remoting/webapp/unittests/menu_button_unittest.js (right):

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/unittes...
remoting/webapp/unittests/menu_button_unittest.js:11: var onHide = sinon.spy();
To avoid side-effect.
We should
var onShow = null;

setup: function () {
  ...
  onShow = sinon.spy()
}

/* optional */
teardown: function () {
  ...
  onShow = null;
}

and likewise for onHide

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/window_...
File remoting/webapp/window_frame.js (right):

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/window_...
remoting/webapp/window_frame.js:108: this.clientSession_ = clientSession;
It seems like clientSession_ is only used as a boolean.

[Jamie, 2014-07-23 20:00:38]

FYI

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/unittes...
File remoting/webapp/unittests/menu_button_unittest.js (right):

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/unittes...
remoting/webapp/unittests/menu_button_unittest.js:11: var onHide = sinon.spy();
On 2014/07/23 18:49:08, kelvinp wrote:
> To avoid side-effect.
> We should
> var onShow = null;
> 
> setup: function () {
>   ...
>   onShow = sinon.spy()
> }
> 
> /* optional */
> teardown: function () {
>   ...
>   onShow = null;
> }
> 
> and likewise for onHide

Good catch! Done.

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/window_...
File remoting/webapp/window_frame.js (right):

https://codereview.chromium.org/339613003/diff/100001/remoting/webapp/window_...
remoting/webapp/window_frame.js:108: this.clientSession_ = clientSession;
On 2014/07/23 18:49:08, kelvinp wrote:
> It seems like clientSession_ is only used as a boolean.

We need a ClientSession in order to set the window title. You're correct that we
don't actually need to store the client session, but as discussed, it doesn't
really make a lot of difference either way so I'm going to leave it as-is.

[Jamie, 2014-07-23 20:00:43]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2014-07-23 20:05:06]

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

[commit-bot: I haz the power, 2014-07-24 01:08:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-24 03:05:24]

Change committed as 285136