Port network options to DOM UI.

Port network options to DOM UI. Hook up all the functionality. UI touchup work will be in another changelist.
BUG=chromium-os:4744
BUG=49030
TEST=manual

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

[Charlie Lee, 2010-07-27 01:32:18]

Can you take a look. I'd like to check this in and then you can take over some
of the work.

[xiyuan, 2010-07-27 16:37:25]

LGTM with nits.

http://codereview.chromium.org/3036020/diff/1/5
File chrome/browser/chromeos/dom_ui/internet_options_handler.h (right):

http://codereview.chromium.org/3036020/diff/1/5#newcode9
chrome/browser/chromeos/dom_ui/internet_options_handler.h:9: #include
"third_party/skia/include/core/SkBitmap.h"
nit: suggest to move SkBitmap.h to cc file and just put a forward declaration
here.

http://codereview.chromium.org/3036020/diff/1/5#newcode26
chrome/browser/chromeos/dom_ui/internet_options_handler.h:26: // Option options
dialog.
nit: "// Open options dialog." ?

http://codereview.chromium.org/3036020/diff/1/10
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/3036020/diff/1/10#newcode6
chrome/browser/resources/options/chromeos_internet_options.js:6: //
SystemOptions class:
nit: InternetOptions class:

http://codereview.chromium.org/3036020/diff/1/10#newcode13
chrome/browser/resources/options/chromeos_internet_options.js:13:
OptionsPage.call(this, 'internet', templateData.internetPage, 'internetPage');
nit: per arv, prefer to  use localStrings.getString('internetPage') instead of
templdateData.

http://codereview.chromium.org/3036020/diff/1/10#newcode35
chrome/browser/resources/options/chromeos_internet_options.js:35:
$('rememberedList').load_(templateData.rememberedList);
nit: trailing underscore means private method. If we want to call it externally,
then no need to add trailing underscore to method names.

[arv (Not doing code reviews), 2010-07-27 18:55:35]

FYI

http://codereview.chromium.org/3036020/diff/1/8
File chrome/browser/resources/options/chromeos_internet_network_list.js (right):

http://codereview.chromium.org/3036020/diff/1/8#newcode22
chrome/browser/resources/options/chromeos_internet_network_list.js:22: decorate:
function() {
I would skip the whole decorate method since you are not using it.

http://codereview.chromium.org/3036020/diff/1/8#newcode53
chrome/browser/resources/options/chromeos_internet_network_list.js:53:
handleChange_: function(e) {
remove if you removed decorate

http://codereview.chromium.org/3036020/diff/1/8#newcode96
chrome/browser/resources/options/chromeos_internet_network_list.js:96:
nameEl.style.fontWeight = 'bold';
Move to CSS?

The following should work

cr.defineProperty(NetworkListItem, 'connected', cr.PropertyKind.BOOL_ATTR);

this will add and remove a connected attribute based on what you set connected
to. Then you can use the following CSS.

#network-list > [connected] {
  font-weight: bold;
}

http://codereview.chromium.org/3036020/diff/1/8#newcode116
chrome/browser/resources/options/chromeos_internet_network_list.js:116:
chrome.send('openOptionsDialog', [String(this.servicePath)]);
wrong this.

var self = this;
optionsButtonEl.onclick = function(event) {
  chrome.send('openOptionsDialog', [String(self.servicePath)]);
};

or

var servicePath = String(this.servicePath);
optionsButtonEl.onclick = function(event) {
  chrome.send('openOptionsDialog', [servicePath]);
};

http://codereview.chromium.org/3036020/diff/1/8#newcode123
chrome/browser/resources/options/chromeos_internet_network_list.js:123: * The ID
of the bookmark folder we are currently showing or loading.
not a bookmark folder :-)

http://codereview.chromium.org/3036020/diff/1/8#newcode128
chrome/browser/resources/options/chromeos_internet_network_list.js:128: },
no trailing commas please

http://codereview.chromium.org/3036020/diff/1/9
File chrome/browser/resources/options/chromeos_internet_options.html (right):

http://codereview.chromium.org/3036020/diff/1/9#newcode5
chrome/browser/resources/options/chromeos_internet_options.html:5: <div>
do you need the div here?

http://codereview.chromium.org/3036020/diff/1/9#newcode11
chrome/browser/resources/options/chromeos_internet_options.html:11: <div>
and here?

http://codereview.chromium.org/3036020/diff/1/9#newcode17
chrome/browser/resources/options/chromeos_internet_options.html:17: <div>
etc

http://codereview.chromium.org/3036020/diff/1/10
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/3036020/diff/1/10#newcode35
chrome/browser/resources/options/chromeos_internet_options.js:35:
$('rememberedList').load_(templateData.rememberedList);
On 2010/07/27 16:37:25, xiyuan wrote:
> nit: trailing underscore means private method. If we want to call it
externally,
> then no need to add trailing underscore to method names.


Or define a NetworkDataModel

list.dataModel = new NetworkDataModel(templateData.rememberedList);

or simply use the ArrayDataModel directly.

list.dataModel = new ArrayDataModel(templateData.rememberedList);

http://codereview.chromium.org/3036020/diff/1/10#newcode38
chrome/browser/resources/options/chromeos_internet_options.js:38:
cr.bind(this.handleVisibleChange_, this));
No need to bind this since you are listening to 'this'.

http://codereview.chromium.org/3036020/diff/1/11
File chrome/browser/resources/options/chromeos_internet_options_page.css
(right):

http://codereview.chromium.org/3036020/diff/1/11#newcode1
chrome/browser/resources/options/chromeos_internet_options_page.css:1: list {
I feel like this might conflict with other lists in the options UI.

One way to solve this is to add a class name to the network lists and use that
in the selectors.

http://codereview.chromium.org/3036020/diff/1/11#newcode25
chrome/browser/resources/options/chromeos_internet_options_page.css:25: right:
3px;
RTL

html[dir=rtl] list > * > button {
  left: 3px;
  right: auto;
}

[Charlie Lee, 2010-07-27 23:05:02]

Ok, made the fixes and added some more functionality code.
arv, I want to check this in so that xiyuan can work on this and help me fix up
the CSS.

xiyuan, all the functionality is now there. I just need you to help me fix up UI
to match the mock as much as possible:
http://folder/alcor/p/Chrome_-_Settings/Settings&sidebar

http://codereview.chromium.org/3036020/diff/1/5
File chrome/browser/chromeos/dom_ui/internet_options_handler.h (right):

http://codereview.chromium.org/3036020/diff/1/5#newcode9
chrome/browser/chromeos/dom_ui/internet_options_handler.h:9: #include
"third_party/skia/include/core/SkBitmap.h"
On 2010/07/27 16:37:25, xiyuan wrote:
> nit: suggest to move SkBitmap.h to cc file and just put a forward declaration
> here.

Done.

http://codereview.chromium.org/3036020/diff/1/5#newcode26
chrome/browser/chromeos/dom_ui/internet_options_handler.h:26: // Option options
dialog.
On 2010/07/27 16:37:25, xiyuan wrote:
> nit: "// Open options dialog." ?

Done.

http://codereview.chromium.org/3036020/diff/1/8
File chrome/browser/resources/options/chromeos_internet_network_list.js (right):

http://codereview.chromium.org/3036020/diff/1/8#newcode22
chrome/browser/resources/options/chromeos_internet_network_list.js:22: decorate:
function() {
On 2010/07/27 18:55:36, arv wrote:
> I would skip the whole decorate method since you are not using it.

Done.

http://codereview.chromium.org/3036020/diff/1/8#newcode53
chrome/browser/resources/options/chromeos_internet_network_list.js:53:
handleChange_: function(e) {
On 2010/07/27 18:55:36, arv wrote:
> remove if you removed decorate

Done.

http://codereview.chromium.org/3036020/diff/1/8#newcode96
chrome/browser/resources/options/chromeos_internet_network_list.js:96:
nameEl.style.fontWeight = 'bold';
I couldn't get this to work like you have it. Xiyuan, can you follow up?

On 2010/07/27 18:55:36, arv wrote:
> Move to CSS?
> 
> The following should work
> 
> cr.defineProperty(NetworkListItem, 'connected', cr.PropertyKind.BOOL_ATTR);
> 
> this will add and remove a connected attribute based on what you set connected
> to. Then you can use the following CSS.
> 
> #network-list > [connected] {
>   font-weight: bold;
> }

http://codereview.chromium.org/3036020/diff/1/8#newcode116
chrome/browser/resources/options/chromeos_internet_network_list.js:116:
chrome.send('openOptionsDialog', [String(this.servicePath)]);
On 2010/07/27 18:55:36, arv wrote:
> wrong this.
> 
> var self = this;
> optionsButtonEl.onclick = function(event) {
>   chrome.send('openOptionsDialog', [String(self.servicePath)]);
> };
> 
> or
> 
> var servicePath = String(this.servicePath);
> optionsButtonEl.onclick = function(event) {
>   chrome.send('openOptionsDialog', [servicePath]);
> };

Done.

http://codereview.chromium.org/3036020/diff/1/8#newcode123
chrome/browser/resources/options/chromeos_internet_network_list.js:123: * The ID
of the bookmark folder we are currently showing or loading.
Deleted.
On 2010/07/27 18:55:36, arv wrote:
> not a bookmark folder :-)

http://codereview.chromium.org/3036020/diff/1/8#newcode128
chrome/browser/resources/options/chromeos_internet_network_list.js:128: },
On 2010/07/27 18:55:36, arv wrote:
> no trailing commas please

Done.

http://codereview.chromium.org/3036020/diff/1/9
File chrome/browser/resources/options/chromeos_internet_options.html (right):

http://codereview.chromium.org/3036020/diff/1/9#newcode5
chrome/browser/resources/options/chromeos_internet_options.html:5: <div>
On 2010/07/27 18:55:36, arv wrote:
> do you need the div here?

Done.

http://codereview.chromium.org/3036020/diff/1/9#newcode11
chrome/browser/resources/options/chromeos_internet_options.html:11: <div>
On 2010/07/27 18:55:36, arv wrote:
> and here?

Done.

http://codereview.chromium.org/3036020/diff/1/9#newcode17
chrome/browser/resources/options/chromeos_internet_options.html:17: <div>
On 2010/07/27 18:55:36, arv wrote:
> etc

Done.

http://codereview.chromium.org/3036020/diff/1/10
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/3036020/diff/1/10#newcode6
chrome/browser/resources/options/chromeos_internet_options.js:6: //
SystemOptions class:
On 2010/07/27 16:37:25, xiyuan wrote:
> nit: InternetOptions class:

Done.

http://codereview.chromium.org/3036020/diff/1/10#newcode13
chrome/browser/resources/options/chromeos_internet_options.js:13:
OptionsPage.call(this, 'internet', templateData.internetPage, 'internetPage');
On 2010/07/27 16:37:25, xiyuan wrote:
> nit: per arv, prefer to  use localStrings.getString('internetPage') instead of
> templdateData.

Done.

http://codereview.chromium.org/3036020/diff/1/10#newcode35
chrome/browser/resources/options/chromeos_internet_options.js:35:
$('rememberedList').load_(templateData.rememberedList);
On 2010/07/27 16:37:25, xiyuan wrote:
> nit: trailing underscore means private method. If we want to call it
externally,
> then no need to add trailing underscore to method names.

Done.

http://codereview.chromium.org/3036020/diff/1/10#newcode35
chrome/browser/resources/options/chromeos_internet_options.js:35:
$('rememberedList').load_(templateData.rememberedList);
On 2010/07/27 18:55:36, arv wrote:
> On 2010/07/27 16:37:25, xiyuan wrote:
> > nit: trailing underscore means private method. If we want to call it
> externally,
> > then no need to add trailing underscore to method names.
> 
> 
> Or define a NetworkDataModel
> 
> list.dataModel = new NetworkDataModel(templateData.rememberedList);
> 
> or simply use the ArrayDataModel directly.
> 
> list.dataModel = new ArrayDataModel(templateData.rememberedList);

Done.

http://codereview.chromium.org/3036020/diff/1/10#newcode38
chrome/browser/resources/options/chromeos_internet_options.js:38:
cr.bind(this.handleVisibleChange_, this));
On 2010/07/27 18:55:36, arv wrote:
> No need to bind this since you are listening to 'this'.

Done.

http://codereview.chromium.org/3036020/diff/1/11
File chrome/browser/resources/options/chromeos_internet_options_page.css
(right):

http://codereview.chromium.org/3036020/diff/1/11#newcode1
chrome/browser/resources/options/chromeos_internet_options_page.css:1: list {
xiyuan, can you follow up on this?

On 2010/07/27 18:55:36, arv wrote:
> I feel like this might conflict with other lists in the options UI.
> 
> One way to solve this is to add a class name to the network lists and use that
> in the selectors.

http://codereview.chromium.org/3036020/diff/1/11#newcode25
chrome/browser/resources/options/chromeos_internet_options_page.css:25: right:
3px;
xiyuan, can you follow up on this?

On 2010/07/27 18:55:36, arv wrote:
> RTL
> 
> html[dir=rtl] list > * > button {
>   left: 3px;
>   right: auto;
> }

[xiyuan, 2010-07-27 23:12:52]

Thanks Charlie.

I'll pick up the CL for DOM UI part after it gets in and will let arv know that
he needs to look for all css/js issues in the next code review.

[Charlie Lee (do not use), 2010-07-27 23:52:55]

arv, let me know if it looks good to you. if so, I will check it in and  
have xiyuan work on the UI.

Thanks,
Charlie

On Jul 27, 2010 4:12:53 PM PDT, xiyuan <xiyuan@chromium.org> wrote:
> Thanks Charlie.
> I'll pick up the CL for DOM UI part after it gets in and will let arv
> know that
> he needs to look for all css/js issues in the next code review.

[arv (Not doing code reviews), 2010-07-28 05:56:53]

LGTM

I still feel like the CSS should be made more specific since this will surely
cause conflicts with other lists on the page.

http://codereview.chromium.org/3036020/diff/14001/15011
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/3036020/diff/14001/15011#newcode64
chrome/browser/resources/options/chromeos_internet_options.js:64:
$('wiredList').redraw();
Setting the data model should redraw the list already.

[Charlie Lee, 2010-07-28 06:45:02]

Ok, I added class="network-list" to the 3 lists in the html and changed the CSS
to reflect. I will check this in. Xiyuan, you can take this over now. Thanks!

http://codereview.chromium.org/3036020/diff/14001/15011
File chrome/browser/resources/options/chromeos_internet_options.js (right):

http://codereview.chromium.org/3036020/diff/14001/15011#newcode64
chrome/browser/resources/options/chromeos_internet_options.js:64:
$('wiredList').redraw();
On 2010/07/28 05:56:53, arv wrote:
> Setting the data model should redraw the list already.

Done.

[Charlie Lee (do not use), 2010-07-28 18:25:32]

After doing a sync, the network page is no longer showing any data. And it  
looks like the accounts page is also busted. Xiyuan, can you look into why  
this is happening?

Thanks,
Charlie

On Jul 27, 2010 11:45:02 PM PDT, chocobo <chocobo@chromium.org> wrote:
> Ok, I added class="network-list" to the 3 lists in the html and changed
> the CSS
> to reflect. I will check this in. Xiyuan, you can take this over now.
> Thanks!

>> http://codereview.chromium.org/3036020/diff/14001/15011
>> File chrome/browser/resources/options/chromeos_internet_options.js
>> (right):
>> http://codereview.chromium.org/3036020/diff/14001/15011#newcode64
>> chrome/browser/resources/options/chromeos_internet_options.js:64:
>> $('wiredList').redraw();
>> On 2010/07/28 05:56:53, arv wrote:
>> Setting the data model should redraw the list already.

> Done.

[xiyuan, 2010-07-28 18:36:05]

I am looking at the issue... It seems to be caused by
window.history.pushState does not return in OptiosnPage's visible setter...

On Wed, Jul 28, 2010 at 11:25 AM, Charlie Lee <chocobo@google.com> wrote:

> After doing a sync, the network page is no longer showing any data. And it
> looks like the accounts page is also busted. Xiyuan, can you look into why
> this is happening?
>
> Thanks,
> Charlie
>
>
> On Jul 27, 2010 11:45:02 PM PDT, chocobo <chocobo@chromium.org> wrote:
>
>> Ok, I added class="network-list" to the 3 lists in the html and changed
>> the CSS
>> to reflect. I will check this in. Xiyuan, you can take this over now.
>> Thanks!
>>
>
>  http://codereview.chromium.org/3036020/diff/14001/15011
>>> File chrome/browser/resources/options/chromeos_internet_options.js
>>> (right):
>>> http://codereview.chromium.org/3036020/diff/14001/15011#newcode64
>>> chrome/browser/resources/options/chromeos_internet_options.js:64:
>>> $('wiredList').redraw();
>>> On 2010/07/28 05:56:53, arv wrote:
>>> Setting the data model should redraw the list already.
>>>
>>
>  Done.
>>
>

[Charlie Lee (do not use), 2010-07-28 18:39:36]

Is it caused by this change?
http://codereview.chromium.org/3017027

- Charlie


On Wed, Jul 28, 2010 at 11:35 AM, Xiyuan Xia <xiyuan@chromium.org> wrote:

> I am looking at the issue... It seems to be caused by
> window.history.pushState does not return in OptiosnPage's visible setter...
>
>
> On Wed, Jul 28, 2010 at 11:25 AM, Charlie Lee <chocobo@google.com> wrote:
>
>> After doing a sync, the network page is no longer showing any data. And it
>> looks like the accounts page is also busted. Xiyuan, can you look into why
>> this is happening?
>>
>> Thanks,
>> Charlie
>>
>>
>> On Jul 27, 2010 11:45:02 PM PDT, chocobo <chocobo@chromium.org> wrote:
>>
>>> Ok, I added class="network-list" to the 3 lists in the html and changed
>>> the CSS
>>> to reflect. I will check this in. Xiyuan, you can take this over now.
>>> Thanks!
>>>
>>
>>  http://codereview.chromium.org/3036020/diff/14001/15011
>>>> File chrome/browser/resources/options/chromeos_internet_options.js
>>>> (right):
>>>> http://codereview.chromium.org/3036020/diff/14001/15011#newcode64
>>>> chrome/browser/resources/options/chromeos_internet_options.js:64:
>>>> $('wiredList').redraw();
>>>> On 2010/07/28 05:56:53, arv wrote:
>>>> Setting the data model should redraw the list already.
>>>>
>>>
>>  Done.
>>>
>>
>