[cros] Network dropdown button in WebUI.

[cros] Network dropdown button in WebUI.
Based on a patch by altimofeev
http://codereview.chromium.org/7461117/

BUG=chromium-os:17892
TEST=manual

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

[Nikita (slow), 2011-07-29 15:48:33]

rhashimoto: Please review menu code refactoring, ready for review.

xiyuan: Please take a look at the WebUI / JS / CSS part.
This CL is based on altimofeev CL. I've added icon support for network icons + a
bit polishing but it's not finished.
Some JS parts might have been improved i.e. style moved from JS to CSS.
I'm planning to do that on Monday as a part of code review just want to get
reviewing started early (today).

Button is used as a dropdown "title" in current implementation w/o icon support.
Style fully reflects select elements.
It might be changed to div with proper styling but I've deprioritized this work
item to get other polishing in.
Keyboard navigation of the control will be improved later.

This CL will go into R14 branch.

[Nikita (slow), 2011-07-29 15:52:11]

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:59: z-index: 0;
This one doesn't help in fact.
I didn't figure out why this hr is still visible through dropdown div elements
while other controls like Continue button is not.
It's not like they don't have background set.

[rhashimoto, 2011-07-29 17:17:13]

I think you'll need sky to approve changes to the MenuItemView API.

Did you consider using a menu model as the common menu definition, either
NetworkMenuModel or ui::MenuModel, instead of adding MenuItemInterface for that
purpose?  I know I was the one who made NetworkMenuModel is-a MenuDelegate and
not-a ui::MenuModel, but it still has an API signature that mostly matches up. 
There is now a MenuModelAdapter that wraps ui::MenuModel in a MenuDelegate
interface for use by MenuItemView.  I could help with that part if it made
sense.

Roy

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:47: public:
virtual destructor?

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:51: const std::wstring& label) = 0;
nit: wrapped indentation needs to be adjusted in this class

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:192: virtual void Cancel();
group overridden methods, add OVERRIDE

[xiyuan, 2011-07-29 21:39:33]

Outside this review and just curious, I know we have once considered Oshima's
webui menu code. Why we decided not to use them?

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:59: z-index: 0;
On 2011/07/29 15:52:11, Nikita Kostylev wrote:
> This one doesn't help in fact.
> I didn't figure out why this hr is still visible through dropdown div elements
> while other controls like Continue button is not.
> It's not like they don't have background set.

You might want to try z-index:1 for the dropdown div.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:513: background-image:
url("../../shared/images/select.png"),
nit: let's use single quote to be consistent in this file.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.js (left):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.js:240: Oobe.getOffset =
function(element) {
Don't delete this function yet. I already have code depends on it. :) It's a
floating bubble and I use this function to align with the anchor element. 

We should consider move this into util.js.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:11: function
DropDown(root, title, items) {
we probably want to put this into a cr.define namespace, either declare one here
or put it with NetworkScreen down below.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:11: function
DropDown(root, title, items) {
It would be better to merge DropDown a HTMLDivElement derived element. I don't
see benefit of separating them.

You could do this by
var DropDown = cr.ui.define('div');

DropDown.prototype  = {
  __proto__: HTMLDivElement,prototype,
  ...
};

Then later to bind an existing div by using DropDown.decorate(existingDiv).

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:14:
this.container_.classList.add('dropdown-container');
this.container_ element seems not used.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:36:
this.root_.removeChild(this.root_.childNodes[1]);
It seems a DropDown has two immediate child: title and items-container. Why we
need to create/delete them whevern there is a change? Could we only create them
once and only update them later?

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:45:
this.root_.removeChild(this.root_.childNodes[1]);
see above. Instead of creating the container everytime. We should only create it
once and we could use innerHTML='' to clear its content here.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:66:
container.style.display = this.isShown ? 'block' : 'none';
suggest to use container.hidden instead of display

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:83: el.innerHTML
= innerHtml;
we probably should try to get rid of this use.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:85: el.link =
this;
nit: why DropDown is called "link"? Why not use parent or controller or
something like that?

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:89:
this.link.isShown = !this.link.isShown;
Do we really toggle isShown here?

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:90:
this.link.root_.childNodes[1].style.display =
use hidden attribute.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:143: var dropdown
= undefined;
we should avoid using global vars. Let's put this as a member var of
NetworkScreen?

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.cc (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:94: delete
last_child_;
why not use scoped_ptr?

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:96: delete previous_;
ditto

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.h (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.h:41: virtual void
OnNetworkManagerChanged(NetworkLibrary* cros);
nit: OVERRIDE

[Nikita (slow), 2011-08-01 08:22:57]

On 2011/07/29 21:39:33, xiyuan wrote:
> Outside this review and just curious, I know we have once considered Oshima's
> webui menu code. Why we decided not to use them?

I think the main reason was that we're pressed on time and decided to go with
something more simplified.
It required significant refactoring from the point of integration with existing
network menu. That code implemented standalone menu and in this case we need
"dropdown" style element.

[Nikita (slow), 2011-08-01 17:07:21]

On 2011/07/29 17:17:13, rhashimoto wrote:
> I think you'll need sky to approve changes to the MenuItemView API.

Added sky.
Scott, what do you think about general direction?
Could this work for R14. Since we're trying to land this thing on R14 branch
there's not much room for a lot of refactoring. Alexey added interface and I
believe it modifies general API in a light way.
We may refactor that in R15.

> 
> Did you consider using a menu model as the common menu definition, either
> NetworkMenuModel or ui::MenuModel, instead of adding MenuItemInterface for
that
> purpose?  I know I was the one who made NetworkMenuModel is-a MenuDelegate and
> not-a ui::MenuModel, but it still has an API signature that mostly matches up.


Alexey did that implementation and I believe doing it other way as you suggest
would require more refactoring which might be done later (in R15) but not for
R14.

> There is now a MenuModelAdapter that wraps ui::MenuModel in a MenuDelegate
> interface for use by MenuItemView.  I could help with that part if it made
> sense.

As I've mentioned above there's not much room, for refactoring since this change
is intended to be merged into R14 branch thus we have to limit number of
changes.

[rhashimoto, 2011-08-01 17:24:45]

On 2011/08/01 17:07:21, Nikita Kostylev wrote:
> On 2011/07/29 17:17:13, rhashimoto wrote:
> > Did you consider using a menu model as the common menu definition, either
> > NetworkMenuModel or ui::MenuModel, instead of adding MenuItemInterface for
> that
> > purpose?  I know I was the one who made NetworkMenuModel is-a MenuDelegate
and
> > not-a ui::MenuModel, but it still has an API signature that mostly matches
up.
> 
> 
> Alexey did that implementation and I believe doing it other way as you suggest
> would require more refactoring which might be done later (in R15) but not for
> R14.

Okay, that sounds like your call.  I made a CL
(http://codereview.chromium.org/7541002/) that (re-)subclasses NetworkMenuModel
from ui::MenuModel just in case.  I won't commit it - you can use it or not as
you see fit.

Roy

[sky, 2011-08-01 21:10:24]

On Mon, Aug 1, 2011 at 10:07 AM,  <nkostylev@chromium.org> wrote:
> On 2011/07/29 17:17:13, rhashimoto wrote:
>>
>> I think you'll need sky to approve changes to the MenuItemView API.
>
> Added sky.
> Scott, what do you think about general direction?

I agree with Roy. We don't need yet another menu interface. Instead
use ui::MenuModel and MenuModelAdapter to bridge to MenuItemView.

  -Scott

> Could this work for R14. Since we're trying to land this thing on R14 branch
> there's not much room for a lot of refactoring. Alexey added interface and I
> believe it modifies general API in a light way.
> We may refactor that in R15.
>
>
>> Did you consider using a menu model as the common menu definition, either
>> NetworkMenuModel or ui::MenuModel, instead of adding MenuItemInterface for
>
> that
>>
>> purpose?  I know I was the one who made NetworkMenuModel is-a MenuDelegate
>> and
>> not-a ui::MenuModel, but it still has an API signature that mostly matches
>> up.
>
>
> Alexey did that implementation and I believe doing it other way as you
> suggest
> would require more refactoring which might be done later (in R15) but not
> for
> R14.
>
>> There is now a MenuModelAdapter that wraps ui::MenuModel in a MenuDelegate
>> interface for use by MenuItemView.  I could help with that part if it made
>> sense.
>
> As I've mentioned above there's not much room, for refactoring since this
> change
> is intended to be merged into R14 branch thus we have to limit number of
> changes.
>
>
> http://codereview.chromium.org/7520037/
>

[Nikita (slow), 2011-08-02 05:00:48]

On 2011/08/01 21:10:24, sky wrote:
> On Mon, Aug 1, 2011 at 10:07 AM,  <mailto:nkostylev@chromium.org> wrote:
> > On 2011/07/29 17:17:13, rhashimoto wrote:
> >>
> >> I think you'll need sky to approve changes to the MenuItemView API.
> >
> > Added sky.
> > Scott, what do you think about general direction?
> 
> I agree with Roy. We don't need yet another menu interface. Instead
> use ui::MenuModel and MenuModelAdapter to bridge to MenuItemView.

Does it mean you're ok with changes in CL that Roy prepared?
http://codereview.chromium.org/7541002/

I think it has to be committed separately then and integrated into R14 branch in
preparation for this CL.

[sky, 2011-08-02 15:27:52]

On 2011/08/02 05:00:48, Nikita Kostylev wrote:
> On 2011/08/01 21:10:24, sky wrote:
> > On Mon, Aug 1, 2011 at 10:07 AM,  <mailto:nkostylev@chromium.org> wrote:
> > > On 2011/07/29 17:17:13, rhashimoto wrote:
> > >>
> > >> I think you'll need sky to approve changes to the MenuItemView API.
> > >
> > > Added sky.
> > > Scott, what do you think about general direction?
> > 
> > I agree with Roy. We don't need yet another menu interface. Instead
> > use ui::MenuModel and MenuModelAdapter to bridge to MenuItemView.
> 
> Does it mean you're ok with changes in CL that Roy prepared?
> http://codereview.chromium.org/7541002/

Yes, I prefer this approach.

  -Scott


> I think it has to be committed separately then and integrated into R14 branch
in
> preparation for this CL.

[Nikita (slow), 2011-08-05 23:40:26]

Xiyuan, please review.

Roy, no need to review, just FYI.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:59: z-index: 0;
On 2011/07/29 21:39:33, xiyuan wrote:
> On 2011/07/29 15:52:11, Nikita Kostylev wrote:
> > This one doesn't help in fact.
> > I didn't figure out why this hr is still visible through dropdown div
elements
> > while other controls like Continue button is not.
> > It's not like they don't have background set.
> 
> You might want to try z-index:1 for the dropdown div.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:513: background-image:
url("../../shared/images/select.png"),
On 2011/07/29 21:39:33, xiyuan wrote:
> nit: let's use single quote to be consistent in this file.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.js (left):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.js:240: Oobe.getOffset =
function(element) {
On 2011/07/29 21:39:33, xiyuan wrote:
> Don't delete this function yet. I already have code depends on it. :) It's a
> floating bubble and I use this function to align with the anchor element. 
> 
> We should consider move this into util.js.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:11: function
DropDown(root, title, items) {
On 2011/07/29 21:39:33, xiyuan wrote:
> we probably want to put this into a cr.define namespace, either declare one
here
> or put it with NetworkScreen down below. 

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:11: function
DropDown(root, title, items) {
On 2011/07/29 21:39:33, xiyuan wrote:
> It would be better to merge DropDown a HTMLDivElement derived element. I don't
> see benefit of separating them.
> 
> You could do this by
> var DropDown = cr.ui.define('div');
> 
> DropDown.prototype  = {
>   __proto__: HTMLDivElement,prototype,
>   ...
> };
> 
> Then later to bind an existing div by using DropDown.decorate(existingDiv).

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:14:
this.container_.classList.add('dropdown-container');
On 2011/07/29 21:39:33, xiyuan wrote:
> this.container_ element seems not used.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:36:
this.root_.removeChild(this.root_.childNodes[1]);
On 2011/07/29 21:39:33, xiyuan wrote:
> It seems a DropDown has two immediate child: title and items-container. Why we
> need to create/delete them whevern there is a change? Could we only create
them
> once and only update them later?

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:45:
this.root_.removeChild(this.root_.childNodes[1]);
On 2011/07/29 21:39:33, xiyuan wrote:
> see above. Instead of creating the container everytime. We should only create
it
> once and we could use innerHTML='' to clear its content here.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:66:
container.style.display = this.isShown ? 'block' : 'none';
On 2011/07/29 21:39:33, xiyuan wrote:
> suggest to use container.hidden instead of display

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:83: el.innerHTML
= innerHtml;
On 2011/07/29 21:39:33, xiyuan wrote:
> we probably should try to get rid of this use.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:85: el.link =
this;
On 2011/07/29 21:39:33, xiyuan wrote:
> nit: why DropDown is called "link"? Why not use parent or controller or
> something like that?

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:89:
this.link.isShown = !this.link.isShown;
On 2011/07/29 21:39:33, xiyuan wrote:
> Do we really toggle isShown here?

Yes, menu is hidden on a click.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:90:
this.link.root_.childNodes[1].style.display =
On 2011/07/29 21:39:33, xiyuan wrote:
> use hidden attribute.

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:143: var dropdown
= undefined;
On 2011/07/29 21:39:33, xiyuan wrote:
> we should avoid using global vars. Let's put this as a member var of
> NetworkScreen?

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.cc (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:94: delete
last_child_;
On 2011/07/29 21:39:33, xiyuan wrote:
> why not use scoped_ptr?

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:96: delete previous_;
On 2011/07/29 21:39:33, xiyuan wrote:
> ditto

Done.

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.h (right):

http://codereview.chromium.org/7520037/diff/7001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.h:41: virtual void
OnNetworkManagerChanged(NetworkLibrary* cros);
On 2011/07/29 21:39:33, xiyuan wrote:
> nit: OVERRIDE

Done.

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
File views/controls/menu/menu_item_view.h (right):

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:47: public:
On 2011/07/29 17:17:13, rhashimoto wrote:
> virtual destructor?

Obsolete, using ui::MenuModel now.

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:51: const std::wstring& label) = 0;
On 2011/07/29 17:17:13, rhashimoto wrote:
> nit: wrapped indentation needs to be adjusted in this class

Obsolete, using ui::MenuModel now.

http://codereview.chromium.org/7520037/diff/7001/views/controls/menu/menu_ite...
views/controls/menu/menu_item_view.h:192: virtual void Cancel();
On 2011/07/29 17:17:13, rhashimoto wrote:
> group overridden methods, add OVERRIDE

Obsolete, using ui::MenuModel now.

[xiyuan, 2011-08-06 00:16:34]

LGTM with nits

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:26: var container
= document.createElement('div')
nit: document -> this.ownerDocument

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:40:
$('dropdownTitle').innerText = title;
nit: innerText -> textContent

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:48: var container
= $('dropdownMenu');
nit: container = this.lastElementChild; And if we use this, then no need to have
id.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:72: if (item.id
== -2) {
nit: give -2 a constant name?

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:74:
itemContentElement = document.createElement('hr');
nit: document -> this.ownerDocument

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:76: var span =
document.createElement('span');
ditto

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:78: var bold =
document.createElement('b');
nit: I would prefer to use CSS instead of create a <b> tag.

span.bold {
  font-weight: bold;
}

and assign 'bold' class if needed.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:90: var
itemElement = document.createElement('div');
nit: document -> this.ownerDocument

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:100: var
wrapperDiv = document.createElement('div');
nit: document -> this.ownerDocument

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:102: var imageDiv
= document.createElement('div');
nit: document -> this.ownerDocument

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:112:
item.controller.childNodes[1].hidden = !item.controller.isShown;
seems like we can make isShown a property.

get isShown() {
  return !this.lastElementChild.hidden;
},
set isShown(show) {
  this.lastElementChild.hidden = !show;
},

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:127: var el =
document.createElement('button');
nit: document -> this.ownerDocument

[Nikita (slow), 2011-08-06 16:41:16]

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:26: var container
= document.createElement('div')
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: document -> this.ownerDocument

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:40:
$('dropdownTitle').innerText = title;
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: innerText -> textContent

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:48: var container
= $('dropdownMenu');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: container = this.lastElementChild; And if we use this, then no need to
have
> id.

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:72: if (item.id
== -2) {
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: give -2 a constant name?

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:74:
itemContentElement = document.createElement('hr');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: document -> this.ownerDocument

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:76: var span =
document.createElement('span');
On 2011/08/06 00:16:34, xiyuan wrote:
> ditto

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:78: var bold =
document.createElement('b');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: I would prefer to use CSS instead of create a <b> tag.
> 
> span.bold {
>   font-weight: bold;
> }
> 
> and assign 'bold' class if needed.

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:90: var
itemElement = document.createElement('div');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: document -> this.ownerDocument

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:100: var
wrapperDiv = document.createElement('div');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: document -> this.ownerDocument

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:102: var imageDiv
= document.createElement('div');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: document -> this.ownerDocument

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:112:
item.controller.childNodes[1].hidden = !item.controller.isShown;
On 2011/08/06 00:16:34, xiyuan wrote:
> seems like we can make isShown a property.
> 
> get isShown() {
>   return !this.lastElementChild.hidden;
> },
> set isShown(show) {
>   this.lastElementChild.hidden = !show;
> },

Done.

http://codereview.chromium.org/7520037/diff/19002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:127: var el =
document.createElement('button');
On 2011/08/06 00:16:34, xiyuan wrote:
> nit: document -> this.ownerDocument

Done.

[xiyuan, 2011-08-06 17:12:17]

still LGTM

Thanks you for making all the changes. :)

http://codereview.chromium.org/7520037/diff/22006/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7520037/diff/22006/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:56:
$('dropdownTitle').textContent = title;
nit: this.firstElementChild?

[Nikita (slow), 2011-08-06 19:27:53]

http://codereview.chromium.org/7520037/diff/22006/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7520037/diff/22006/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:56:
$('dropdownTitle').textContent = title;
On 2011/08/06 17:12:17, xiyuan wrote:
> nit: this.firstElementChild?

Done, I've thought that Chrome doesn't support this yet.