Add network_menu_ui.cc and network_menu.js

Add network_menu_ui.cc and network_menu.js
Implement the network menu as a subclass of MenuUI (c++) and Menu (js).

BUG=chromium-os:7343
TEST=Test that the network menu is still functional. NOTE: With this CL, the menu won't look pretty, and it still has the old behavior. There will be a followup CL that will fix the UI.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62266

[stevenjb, 2010-10-11 19:06:08]

Menu doesn't look pretty yet, but the info is there.

The hook for HandleAction() is there, but the actions still need to be defined.

[stevenjb, 2010-10-11 22:54:14]

Update with localized messages.

[xiyuan, 2010-10-11 23:19:26]

http://codereview.chromium.org/3675002/diff/3001/4001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3675002/diff/3001/4001#newcode9852
chrome/app/generated_resources.grd:9852: Connected
nit: remove the "tab" char.

http://codereview.chromium.org/3675002/diff/3001/4006
File chrome/browser/chromeos/dom_ui/network_menu_ui.cc (right):

http://codereview.chromium.org/3675002/diff/3001/4006#newcode58
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:58: std::string ip_address;
nuke the above 4 unused local vars.

http://codereview.chromium.org/3675002/diff/3001/4008
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3675002/diff/3001/4008#newcode49
chrome/browser/chromeos/status/network_menu.cc:49: info->network_type =
"ethernet";
please use constants instead of string literals for network_type, status etc.

http://codereview.chromium.org/3675002/diff/3001/4008#newcode69
chrome/browser/chromeos/status/network_menu.cc:69: // TODO(stevenjb): Eliminate
verbose status message, or localize properly.
please wrap the long lines here and line 71, 97, 100

http://codereview.chromium.org/3675002/diff/3001/4008#newcode120
chrome/browser/chromeos/status/network_menu.cc:120: found = true;
did you mean "found = false;"?

http://codereview.chromium.org/3675002/diff/3001/4009
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/3675002/diff/3001/4009#newcode55
chrome/browser/chromeos/status/network_menu.h:55: std::string ip_address;    //
IP address (if network is active, empty otherwise)
line 53, 54, 55 is longer than 80 chars, please wrap properly.

http://codereview.chromium.org/3675002/diff/3001/4011
File chrome/browser/resources/menu.html (right):

http://codereview.chromium.org/3675002/diff/3001/4011#newcode9
chrome/browser/resources/menu.html:9: <script src="network_menu.js"></script>
We should not be doing this. I think Oshima has a way to load js for derived
DOMUI menu. Check wrench_menu_ui.cc.

[oshima, 2010-10-12 00:14:37]

http://codereview.chromium.org/3675002/diff/3001/4004
File chrome/browser/chromeos/dom_ui/menu_ui.cc (right):

http://codereview.chromium.org/3675002/diff/3001/4004#newcode422
chrome/browser/chromeos/dom_ui/menu_ui.cc:422: &MenuHandler::HandleAction));
please move subclass specific JS action to subclass. You can define your message
handler and add in sublcass. Please see
my wrench menu as an example.

http://codereview.chromium.org/3675002/diff/3001/4007
File chrome/browser/chromeos/dom_ui/network_menu_ui.h (right):

http://codereview.chromium.org/3675002/diff/3001/4007#newcode28
chrome/browser/chromeos/dom_ui/network_menu_ui.h:28: 
no space for the group of methods that "overrides"

http://codereview.chromium.org/3675002/diff/3001/4009
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/3675002/diff/3001/4009#newcode14
chrome/browser/chromeos/status/network_menu.h:14: #include
"views/controls/menu/menu_2.h"
since you changed to scoped_ptr, Menu2 can be forwared decl.

http://codereview.chromium.org/3675002/diff/3001/4011
File chrome/browser/resources/menu.html (right):

http://codereview.chromium.org/3675002/diff/3001/4011#newcode9
chrome/browser/resources/menu.html:9: <script src="network_menu.js"></script>
On 2010/10/11 23:19:27, xiyuan wrote:
> We should not be doing this. I think Oshima has a way to load js for derived
> DOMUI menu. Check wrench_menu_ui.cc.

Yes, please use resource id to inject the code to this template.

http://codereview.chromium.org/3675002/diff/3001/4012
File chrome/browser/resources/network_menu.js (right):

http://codereview.chromium.org/3675002/diff/3001/4012#newcode10
chrome/browser/resources/network_menu.js:10: }
this is already defined in menu.js, so you don't need this.

http://codereview.chromium.org/3675002/diff/3001/4012#newcode21
chrome/browser/resources/network_menu.js:21: initMenuItem_:
function(leftIconWidth) {
this parameter has changed a bit in my cl 3601021 and depending on which one
goes in first, we need to update this.

http://codereview.chromium.org/3675002/diff/3001/4012#newcode23
chrome/browser/resources/network_menu.js:23: //
MenuItem.prototype.initMenuItem_(leftIconWidth);
You can call super class's method like this (or something like this). Please
confirm with xiyuan.

MenuItem.prototype.initMenuItem_.call(this, leftIconWidth);

http://codereview.chromium.org/3675002/diff/3001/4012#newcode67
chrome/browser/resources/network_menu.js:67: }
we need to clean up this part. xiyuan, I assume you takes care of it?

http://codereview.chromium.org/3675002/diff/3001/4012#newcode96
chrome/browser/resources/network_menu.js:96: 
remove extra line

[stevenjb, 2010-10-12 02:45:57]

http://codereview.chromium.org/3675002/diff/3001/4001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3675002/diff/3001/4001#newcode9852
chrome/app/generated_resources.grd:9852: Connected
On 2010/10/11 23:19:27, xiyuan wrote:
> nit: remove the "tab" char.

Done.

http://codereview.chromium.org/3675002/diff/3001/4006
File chrome/browser/chromeos/dom_ui/network_menu_ui.cc (right):

http://codereview.chromium.org/3675002/diff/3001/4006#newcode58
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:58: std::string ip_address;
On 2010/10/11 23:19:27, xiyuan wrote:
> nuke the above 4 unused local vars.

Done.

http://codereview.chromium.org/3675002/diff/3001/4008
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3675002/diff/3001/4008#newcode49
chrome/browser/chromeos/status/network_menu.cc:49: info->network_type =
"ethernet";
On 2010/10/11 23:19:27, xiyuan wrote:
> please use constants instead of string literals for network_type, status etc.

Done.

http://codereview.chromium.org/3675002/diff/3001/4008#newcode69
chrome/browser/chromeos/status/network_menu.cc:69: // TODO(stevenjb): Eliminate
verbose status message, or localize properly.
On 2010/10/11 23:19:27, xiyuan wrote:
> please wrap the long lines here and line 71, 97, 100

Done.

http://codereview.chromium.org/3675002/diff/3001/4008#newcode120
chrome/browser/chromeos/status/network_menu.cc:120: found = true;
On 2010/10/11 23:19:27, xiyuan wrote:
> did you mean "found = false;"?

No - we set the menu item visibility based on 'found', so we want it to be true.
This is confusing so I've changed the logic.

http://codereview.chromium.org/3675002/diff/3001/4009
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/3675002/diff/3001/4009#newcode14
chrome/browser/chromeos/status/network_menu.h:14: #include
"views/controls/menu/menu_2.h"
On 2010/10/12 00:14:37, oshima wrote:
> since you changed to scoped_ptr, Menu2 can be forwared decl.

Done.

http://codereview.chromium.org/3675002/diff/3001/4009#newcode55
chrome/browser/chromeos/status/network_menu.h:55: std::string ip_address;    //
IP address (if network is active, empty otherwise)
On 2010/10/11 23:19:27, xiyuan wrote:
> line 53, 54, 55 is longer than 80 chars, please wrap properly.

Done.

http://codereview.chromium.org/3675002/diff/3001/4011
File chrome/browser/resources/menu.html (right):

http://codereview.chromium.org/3675002/diff/3001/4011#newcode9
chrome/browser/resources/menu.html:9: <script src="network_menu.js"></script>
On 2010/10/12 00:14:37, oshima wrote:
> On 2010/10/11 23:19:27, xiyuan wrote:
> > We should not be doing this. I think Oshima has a way to load js for derived
> > DOMUI menu. Check wrench_menu_ui.cc.
> 
> Yes, please use resource id to inject the code to this template.
> 
> 

Done.

http://codereview.chromium.org/3675002/diff/3001/4012
File chrome/browser/resources/network_menu.js (right):

http://codereview.chromium.org/3675002/diff/3001/4012#newcode10
chrome/browser/resources/network_menu.js:10: }
On 2010/10/12 00:14:37, oshima wrote:
> this is already defined in menu.js, so you don't need this.
This is "sendAction", not "sendActivate".

http://codereview.chromium.org/3675002/diff/3001/4012#newcode23
chrome/browser/resources/network_menu.js:23: //
MenuItem.prototype.initMenuItem_(leftIconWidth);
On 2010/10/12 00:14:37, oshima wrote:
> You can call super class's method like this (or something like this). Please
> confirm with xiyuan.
> 
> MenuItem.prototype.initMenuItem_.call(this, leftIconWidth);

Just a placeholder comment indicating that we should eventually avoid copy/paste
code, not meant to be syntactically correct.

http://codereview.chromium.org/3675002/diff/3001/4012#newcode67
chrome/browser/resources/network_menu.js:67: }
On 2010/10/12 00:14:37, oshima wrote:
> we need to clean up this part. xiyuan, I assume you takes care of it?

That's the plan :)

http://codereview.chromium.org/3675002/diff/3001/4012#newcode96
chrome/browser/resources/network_menu.js:96: 
On 2010/10/12 00:14:37, oshima wrote:
> remove extra line

Done.

[oshima, 2010-10-12 03:37:48]

lgtm c++ side.

http://codereview.chromium.org/3675002/diff/38001/39004
File chrome/browser/chromeos/dom_ui/network_menu_ui.cc (right):

http://codereview.chromium.org/3675002/diff/38001/39004#newcode60
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:60: } // namespace
2 space between

http://codereview.chromium.org/3675002/diff/38001/39004#newcode87
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:87: //const NetworkMenu*
network_menu = static_cast<const NetworkMenu*>(model);
nuke dead code.

http://codereview.chromium.org/3675002/diff/38001/39004#newcode91
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:91: DictionaryValue*
NetworkMenuUI::CreateMenuItem(const menus::MenuModel* model,
just FYI: i'm going to change to reference (&)

[xiyuan, 2010-10-12 16:28:53]

LGTM

I'll touch up the html/js part once this change gets in.