Fix favorite and auto_connect property usage.

Fix favorite and auto_connect property usage.

BUG=chromium-os:8913
TEST=manually open chrome://settings/internet and see if remembered networks are listed properly.  For DOMUI network menu, pass --enable-domui-menu for chrome to enable and check.

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

[kochi, 2010-11-12 12:13:16]

Hi Charlie,

This is from what we discussed a while ago in chromium-os-dev,
about favorite vs auto_connect property.

I don't think this will make visible change at this moment
(as both properties are handled almost interchangeably),
but I believe this is more correct usage.

Could you review this?

[Charlie Lee, 2010-11-12 12:29:57]

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/cros/ne...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/cros/ne...
chrome/browser/chromeos/cros/network_library.h:134: // No setter for favorite_.
Please explain why. We don't have a setter for favorite because to unfavorite a
network is equivalent to forget a network, so we call forget network on cros for
that.

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/network_menu.cc:203: int remember) const {
why is this an int? and is this misnamed? should it be auto-connect instead?
When we connect to a new network, we give user an option to auto-connect or not,
right?

who calls this method?

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/network_menu.cc:211: if (remember >= 0)
if remember means auto-connect, then this is the right thing to do.

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/network_menu.cc:281: bool favorite = remember ==
0 ? false : true;  // default is true
rename this to auto-connect?

[kochi, 2010-11-15 08:46:13]

Hi,

Thanks for the comments.

I don't like to introduce new changes during a review process, but one renaming
caused another again and again...
The diff grew, but mostly simple renaming of s/remember/autoconnect/.

Sorry about this, but could you review?

Thanks,

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/cros/ne...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/cros/ne...
chrome/browser/chromeos/cros/network_library.h:134: // No setter for favorite_.
adopted your comment.
thanks!

On 2010/11/12 12:29:57, Charlie Lee wrote:
> Please explain why. We don't have a setter for favorite because to unfavorite
a
> network is equivalent to forget a network, so we call forget network on cros
for
> that.

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/network_menu.cc:203: int remember) const {
I think 'int' is because it is adapted to the callback from
DOMUI JavaScript network menu code, which is disabled for now.

I did more complete renaming of s/remember/autocomplete/ now.

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/network_menu.cc:211: if (remember >= 0)
Renamed to auto_connect.

On 2010/11/12 12:29:57, Charlie Lee wrote:
> if remember means auto-connect, then this is the right thing to do.

http://codereview.chromium.org/4818002/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/network_menu.cc:281: bool favorite = remember ==
0 ? false : true;  // default is true
On 2010/11/12 12:29:57, Charlie Lee wrote:
> rename this to auto-connect?

Done.

[Charlie Lee, 2010-11-15 19:42:50]

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/network_menu_ui.cc (left):

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:34:
l10n_util::GetStringUTF16(IDS_NETWORK_REMEMBER_THIS_NETWORK_TITLE));
If this string is no longer needed, please remove it.

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/resources/net...
File chrome/browser/resources/network_menu.js (right):

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/resources/net...
chrome/browser/resources/network_menu.js:146: if (this.attrs.auto_connect) {
I think this logic here needs to be updated. We do want to use remembered here.
The logic is if the network is remembered, then we don't need to pass back the
password. Maybe we just pass the password all the time even if it is remembered?
How does the code handle this?

[kochi, 2010-11-16 05:47:23]

Thanks for the review.

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/network_menu_ui.cc (left):

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/network_menu_ui.cc:34:
l10n_util::GetStringUTF16(IDS_NETWORK_REMEMBER_THIS_NETWORK_TITLE));
No one else is using.
Removed.

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/resources/net...
File chrome/browser/resources/network_menu.js (right):

http://codereview.chromium.org/4818002/diff/6001/chrome/browser/resources/net...
chrome/browser/resources/network_menu.js:146: if (this.attrs.auto_connect) {
Good catch.
If the network is remembered, we do not show any input
boxes to prompt password etc.  So in this case we do not
have auto_connect checkbox anyway, so we just pass the password
and auto_connect attribute as is.

[Charlie Lee, 2010-11-16 10:26:09]

LGTM