Added "Use an autoconfiguration URL" checkbox in proxy tab

Added "Use an autoconfiguration URL" checkbox in proxy tab
    
    Adding this checkbox makes user to be aware of auto detecting proxy
    option and also meets the new design suggested to solve the same.
    Providing this checkbox also ease the user to select between auto
    configuring proxy(PAC) and auto detecting proxy(WPAD). This
    checkbox functions similar to the one already provided to select
    single proxy mode.
    
    Contributed by sathish.kuppuswamy@intel.com
    
    BUG=chromium:222576
TEST=Built the changes along with Chromium OS, verified the
         behaviour of the checkbox and proxy connection. Selected
         "Automatic proxy configuration" option
         1, Verified internet access after checking "use a
         autoconfiguration URL" checkbox and entering a valid PAC
         URL in the URL field(Auto configuring proxy).
         2, Verified internet access, leaving "use a autoconfiguration
         URL" unchecked(Auto Detecting proxy). Same is been verified by
         checking "use a autoconfiguration URL" checkbox, but leaving
         the URL field blank.

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

[Dan Beam, 2013-03-29 22:23:34]

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/app/chromeos_st...
File chrome/app/chromeos_strings.grdp (right):

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/app/chromeos_st...
chrome/app/chromeos_strings.grdp:2354: <message name="IDS_PROXY_CONFIG_URL"
desc="Label for the proxy config URL box.">
you should probably rename this and update the desc as well, i.e.
IDS_PROXY_USE_AUTOCONFIG_URL and "Label for the checkbox that controls whether
to use an autoconfiguration URL."

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:240: if (val) {
if (val && config.automatic_proxy.pac_url.is_valid()) {
  config_service->UISetProxyConfigToPACScript(
      config.automatic_proxy.pac_url);
} else {
  config_service->UISetProxyConfigToAutoDetect();
}

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:241: if
(config.automatic_proxy.pac_url.is_valid())
nit: please use {curlies} in this if/else (even though bad code around here
doesn't)

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:244: else
nit: indent off

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:383: 
nit: please remove this \n

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.html (right):

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.html:656:
pref="cros.session.proxy.usepacurl">
nit: add 3 more \s here, i.e.

  <input id="proxy-config-url" type="checkbox"
      pref="cros.session.proxy.usepacurl">

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:469:
$('proxy-config-url').disabled = $('auto-proxy').disabled ||
nit: can you put proxy-config-url second, i.e.

  $('proxy-config').disabled = $('proxy-config-url').disabled ||
                               !$('proxy-config-url').checked;
  $('proxy-config-url').disabled = $('auto-proxy').disabled ||
                                   !$('auto-proxy').checked;

https://chromiumcodereview.appspot.com/13334007/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/chromeos/internet_detail.js:499:
$('proxy-config-url').disabled = true;
same here, can you put proxy-config-url after, like this:

  $('proxy-config').disabled = true;
  $('proxy-config-url').disabled = true;

[sathish.kuppuswamy, 2013-03-29 22:59:05]

chrome/browser/resources/options/chromeos/internet_detail.js:469:
 $('proxy-config-url').disabled = $('auto-proxy').disabled ||
 nit: can you put proxy-config-url second, i.e.
 
   $('proxy-config').disabled = $('proxy-config-url').disabled ||
                                !$('proxy-config-url').checked;
   $('proxy-config-url').disabled = $('auto-proxy').disabled ||
                                    !$('auto-proxy').checked;
 
Dan,
  I have a question in the above review comment.
  Here, "proxy-config-url" is the checkbox and "proxy-config" is the textbox for
the URL. By name, they convey vice-versa. Do you still want it to be reversed?

[Dan Beam, 2013-03-29 23:42:12]

On 2013/03/29 22:59:05, sathish.kuppuswamy wrote:
> chrome/browser/resources/options/chromeos/internet_detail.js:469:
>  $('proxy-config-url').disabled = $('auto-proxy').disabled ||
>  nit: can you put proxy-config-url second, i.e.
>  
>    $('proxy-config').disabled = $('proxy-config-url').disabled ||
>                                 !$('proxy-config-url').checked;
>    $('proxy-config-url').disabled = $('auto-proxy').disabled ||
>                                     !$('auto-proxy').checked;
>  
> Dan,
>   I have a question in the above review comment.
>   Here, "proxy-config-url" is the checkbox and "proxy-config" is the textbox
for
> the URL. By name, they convey vice-versa. Do you still want it to be reversed?

Oh, sorry, if I just reversed the names incorrectly.  No, you should keep their
original reasons to be disabled but just put them in the new order of

  $('proxy-config').disabled =
  $('proxy-config-url').disabled =

[Dan Beam, 2013-04-04 00:53:59]

https://chromiumcodereview.appspot.com/13334007/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://chromiumcodereview.appspot.com/13334007/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:238: bool val;
nit: can you use a more descriptive name here like

  bool use_pac_url;

?

https://chromiumcodereview.appspot.com/13334007/diff/9001/chrome/browser/reso...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://chromiumcodereview.appspot.com/13334007/diff/9001/chrome/browser/reso...
chrome/browser/resources/options/chromeos/internet_detail.js:446:
$('proxy-config').textContent = '';
^ hmmm, why do you want to clear the text content?

https://chromiumcodereview.appspot.com/13334007/diff/9001/chrome/browser/reso...
chrome/browser/resources/options/chromeos/internet_detail.js:472:
!$('auto-proxy').checked;
nit: line this up like this:

  $('proxy-config-url').disabled = $('auto-proxy').disabled ||
                                   !$('auto-proxy').checked;

[Dan Beam, 2013-04-04 00:54:31]

by the way, when you update your code, make sure to publish and mail me a
comment saying you've updated it

[sathish.kuppuswamy, 2013-04-05 00:20:06]

Hi Dan,
   Sorry for not updating earlier. I thought it would have triggered mail
automatically. Apart from that, fixed your review comments and updated patch set
3. 
   For your question on clearing the URL field in patch set 2, I thought
clearing the text when check box is unchecked would be better but later from
your review comment felt that leaving the text till window refreshes will be
good for user to come back and edit.

[Dan Beam, 2013-04-05 02:51:17]

please re-upload, something about your last upload failed :(

[sathish.kuppuswamy, 2013-04-05 20:31:39]

Re-Uploaded. Review Patch set-4

[Dan Beam, 2013-04-06 01:45:14]

+nkostylev@ as he's an OWNER of chrome/browser/chromeos (and a more local owner
than I everywhere else).

how have you been testing this (either automatedly or manually)?

also, i noticed that if set a proxy PAC URL and click

  (X) Direct Internet connection

and back to

  (x) Automatic proxy configuration

the proxy PAC URL is still [disabled] (see: http://i.imgur.com/Ir9fD3y.png)

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:49: kProxyUsePacUrl
nit: ,

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.html (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.html:656:
pref="cros.session.proxy.usepacurl">
I think you need to register this pref? +nkostylev@, does he?

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:441:
toggleAutoConfigProxy_: function(e) {
$('proxy-config').disabled = !$('proxy-config-url').checked;

[Nikita (slow), 2013-04-08 17:17:30]

+Steven who's a better reviewer for this change.

[stevenjb, 2013-04-08 18:52:03]

+ gspencer@chromium.org, pneubeck@chromium.org, who are more familiar with proxy
and settings UI code than I am.

[Greg Spencer (Chromium), 2013-04-08 19:55:21]

On 2013/04/08 18:52:03, stevenjb (chromium) wrote:
> + mailto:gspencer@chromium.org, mailto:pneubeck@chromium.org, who are more
familiar with proxy
> and settings UI code than I am.

Honestly, I don't think anyone is familiar with this code.

[Greg Spencer (Chromium), 2013-04-08 20:07:32]

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:468:
$('proxy-config').disabled = $('proxy-config-url').disabled ||
Isn't this redundant? (because disabled == !checked)

[sathish.kuppuswamy, 2013-04-08 21:32:01]

On 2013/04/08 21:16:29, sathish.kuppuswamy wrote:
Dan, 
 The reason it didn't work is because of the change we did as part of review
comments,

$('proxy-config').disabled = $('proxy-config-url').disabled ||
!$('proxy-config-url').checked;
$('proxy-config-url').disabled = $('auto-proxy').disabled ||
!$('auto-proxy').checked;

 Here, proxy-config which is the URL field checks the status of proxy-config-url
which is the checkbox. With the scenario you were mentioning, proxy-config-url
is disabled and checked, when we are back to auto-proxy from direct. This keeps
the URL field disabled as condition  $('proxy-config-url').disabled is true.
Later in the next line, Proxy-config-URl checks the status of "Auto0proxy' radio
button and gets enabled.
  Reverting the order of these lines back will fix this problem.

[sathish.kuppuswamy, 2013-04-08 21:46:43]

Greg,
   Atleast for the "proxy-config-url" checkbox field, removing the disabled may
cause failure at particular scenario.
    When, "Proxy-config-url" is checked and disabled (Switch to direct from auto
proxy), 

Changing the below line from
$('proxy-config').disabled = $('proxy-config-url').disabled ||
!$('proxy-config-url').checked;
to
$('proxy-config').disabled = !$('proxy-config-url').checked;
   will make "proxy-config".disabled=false which will enable the URL field
though the check box "Use an autoconfiguration URL" disabled.

Correct ,me if my understanding is wrong. Although I feel you are right with the
other line

$('proxy-config-url').disabled = $('auto-proxy').disabled ||
!$('auto-proxy').checked;

But I retained it as it was existing code.

[sathish.kuppuswamy, 2013-04-08 21:48:52]

Dan, 
 To answer your question regarding testing. I did it manually.

[Dan Beam, 2013-04-09 04:28:43]

On 2013/04/08 21:32:01, sathish.kuppuswamy wrote:
> On 2013/04/08 21:16:29, sathish.kuppuswamy wrote:
> Dan, 
>  The reason it didn't work is because of the change we did as part of review
> comments,
> 
> $('proxy-config').disabled = $('proxy-config-url').disabled ||
> !$('proxy-config-url').checked;
> $('proxy-config-url').disabled = $('auto-proxy').disabled ||
> !$('auto-proxy').checked;
> 
>  Here, proxy-config which is the URL field checks the status of
proxy-config-url
> which is the checkbox. With the scenario you were mentioning, proxy-config-url
> is disabled and checked, when we are back to auto-proxy from direct. This
keeps
> the URL field disabled as condition  $('proxy-config-url').disabled is true.
> Later in the next line, Proxy-config-URl checks the status of "Auto0proxy'
radio
> button and gets enabled.
>   Reverting the order of these lines back will fix this problem.

Oh, I didn't know the order mattered here.  Sorry, yeah, change them back.

[pneubeck (no reviews), 2013-04-09 09:48:19]

Overall, I don't like modifying this deprecated (because hard to maintain) proxy
UI code. We could as well realize a new UI during the upcoming reimplementation
and spare ourselves the double work.

If you still want to complete it now, see my comments please.

Please also update commit message:
- The description could mention what this checkbox does (switch between WPAD and
PAC).
- The 'TEST=' field should describe _how_ to test this.

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:32: const char
kProxyUsePacUrl[]      = "cros.session.proxy.usepacurl";
I don't like this forwarding of every UI element's value to C++ but rather keep
as many details locally in JS as possible.

What you have in the UI is a two level nesting of conditionals:
  'type (direct/manual/auto)'
  if 'type == auto' -> 'use pac URL'
  if 'use pac URL' -> 'pac URL'

but in C++ there is only one conditional: 'type (direct/manual/PAC/WPAD)'.

I'd very much prefer, if you did the mapping in JS and not in C++.
This would in particular allow to verify the URL (at least check for the input
field to be non-empty) at first within JS before sending pref changes to C++.

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:136: if
(in_value->GetAsString(&val)) {
Why not check for the checkbox's value?

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.html (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.html:655: <input
id="proxy-config-url" type="checkbox"
please change this id to better reflect the pref, like
"proxy-config-use-pac-url"

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.html:662: <input
id="proxy-config" type="url" size="50"
please update this id to something more reasonable like "proxy-config-pac-url"

This misnaming already lead to the misunderstandings about the updating of the
.disabled attributes in the .js.

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:441:
toggleAutoConfigProxy_: function(e) {
"toggle" sounds to me like it switches between two states on each call, which
isn't the case here.
If it's handling something then better call it "handleAutoConfigProxy_" or
"on..." or "update...".

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:468:
$('proxy-config').disabled = $('proxy-config-url').disabled ||
These lines are only confusing. Why would you ask for
$('proxy-config-url').disabled if you set it afterwards.
Logically, you should
- at first update 'proxy-config-url' as it controls the text field
- then update 'proxy-config'

[sathish.kuppuswamy, 2013-04-16 16:46:57]

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:32: const char
kProxyUsePacUrl[]      = "cros.session.proxy.usepacurl";
Hi pneubeck,
   Here(.js) you want me to check the URL field is empty or not, when usePacUrl
checkbox is checked and send a preference commit call to save the same to
backend with this call
Preferences.setURLPref(pref,Value,true);  ?
 I didn't dig deep into working of preferences. I just made sure this works
similar to other checkbox(use same proxy) in proxy tab .

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:136: if
(in_value->GetAsString(&val)) {
Hi pneubeck,
    Its been checked below (path== kProxyUsePacUrl). Removing this above
condition prevented setting PAC URL to proxy Config, which in turn made to fall
always to autodetect.

[sathish.kuppuswamy, 2013-04-16 23:05:43]

Fixed all review comments except comments related to passing preference to C++ .

Other than that, the tested functionality working manually for auto proxy and
auto detection. And also tested the enable/disable scenario's for "use a
autoconfiguration URL" checkbox.
I renamed checkbox id as "proxy-use-pac-url" and URL field as "proxy-pac-url". I
didn't go for "proxy-config-use-pac-url" beacause of 80 character/line
restrictions.

[sathish.kuppuswamy, 2013-04-16 23:06:40]

Updated patch set 5. Please do the code-review.

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.html (right):

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.html:655: <input
id="proxy-config-url" type="checkbox"
On 2013/04/09 09:48:19, pneubeck wrote:
> please change this id to better reflect the pref, like
> "proxy-config-use-pac-url"

Done.

https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.html:662: <input
id="proxy-config" type="url" size="50"
On 2013/04/09 09:48:19, pneubeck wrote:
> please update this id to something more reasonable like "proxy-config-pac-url"
> 
> This misnaming already lead to the misunderstandings about the updating of the
> .disabled attributes in the .js.

Done.

[pneubeck (no reviews), 2013-04-17 07:20:25]

Please update the issue description as mentioned in my previous comment. ("Edit
Issue" at the top left)

On 2013/04/16 23:06:40, sathish.kuppuswamy wrote:
> Updated patch set 5. Please do the code-review.
> 
>
https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
> File chrome/browser/resources/options/chromeos/internet_detail.html (right):
> 
>
https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
> chrome/browser/resources/options/chromeos/internet_detail.html:655: <input
> id="proxy-config-url" type="checkbox"
> On 2013/04/09 09:48:19, pneubeck wrote:
> > please change this id to better reflect the pref, like
> > "proxy-config-use-pac-url"
> 
> Done.
> 
>
https://codereview.chromium.org/13334007/diff/31001/chrome/browser/resources/...
> chrome/browser/resources/options/chromeos/internet_detail.html:662: <input
> id="proxy-config" type="url" size="50"
> On 2013/04/09 09:48:19, pneubeck wrote:
> > please update this id to something more reasonable like
"proxy-config-pac-url"
> > 
> > This misnaming already lead to the misunderstandings about the updating of
the
> > .disabled attributes in the .js.
> 
> Done.

[sathish.kuppuswamy, 2013-04-17 18:28:46]

Updated the description and test field.

[sathish.kuppuswamy, 2013-04-17 18:30:19]

Updated issue description and test field.

[pneubeck (no reviews), 2013-04-17 20:31:59]

LGTM. I think, with regard to proxy settings it should be fine.
You still need owners lgtms.

[Dan Beam, 2013-04-17 20:55:50]

very very close, when was the last time you updated your source checkout?  I'm
having problems patching in your changes locally.  Please update your code with
gclient sync and reupload (you'll need to do this before you can land either
way).

https://codereview.chromium.org/13334007/diff/50001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://codereview.chromium.org/13334007/diff/50001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:242:
config.automatic_proxy.pac_url);
nit: remove one space

https://codereview.chromium.org/13334007/diff/50001/chrome/browser/chromeos/p...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:353: data =
base::Value::CreateBooleanValue(config.mode ==
nit: remove one space from both of these lines

https://codereview.chromium.org/13334007/diff/50001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/13334007/diff/50001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:264:
$('proxy-use-pac-url').addEventListener('click',
nit: click -> change

https://codereview.chromium.org/13334007/diff/50001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:446: }
$('proxy-pac-url').disabled = !$('proxy-use-pac-url').checked;

[Dan Beam, 2013-04-17 21:02:09]

On 2013/04/17 20:55:50, Dan Beam wrote:
> very very close, when was the last time you updated your source checkout?  I'm
> having problems patching in your changes locally.  Please update your code
with
> gclient sync and reupload (you'll need to do this before you can land either
> way).

Never mind, got the patch applied -- I had a previous version of your CL already
committed, heh, sorry.

> 
>
https://codereview.chromium.org/13334007/diff/50001/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):
> 
>
https://codereview.chromium.org/13334007/diff/50001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/proxy_cros_settings_parser.cc:242:
> config.automatic_proxy.pac_url);
> nit: remove one space
> 
>
https://codereview.chromium.org/13334007/diff/50001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/proxy_cros_settings_parser.cc:353: data =
> base::Value::CreateBooleanValue(config.mode ==
> nit: remove one space from both of these lines
> 
>
https://codereview.chromium.org/13334007/diff/50001/chrome/browser/resources/...
> File chrome/browser/resources/options/chromeos/internet_detail.js (right):
> 
>
https://codereview.chromium.org/13334007/diff/50001/chrome/browser/resources/...
> chrome/browser/resources/options/chromeos/internet_detail.js:264:
> $('proxy-use-pac-url').addEventListener('click',
> nit: click -> change
> 
>
https://codereview.chromium.org/13334007/diff/50001/chrome/browser/resources/...
> chrome/browser/resources/options/chromeos/internet_detail.js:446: }
> $('proxy-pac-url').disabled = !$('proxy-use-pac-url').checked;

[sathish.kuppuswamy, 2013-04-17 22:59:27]

Uploaded Patch set 6, fixing Dan's review comments.

Dan,
 Apart from this issue, I am curious to know about the new UI for proxy window,
about which pneubeck was mentioning earlier. Are you going to be Owner for that?
I will be glad to work on that.

[Dan Beam, 2013-04-17 23:11:42]

https://codereview.chromium.org/13334007/diff/65001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/13334007/diff/65001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/internet_detail.js:447: }
now remove L443-447

[Dan Beam, 2013-04-17 23:21:20]

pneubeck@: should any of the proxy settings be updated if another settings page
changes them?  (i.e. settings page in window 1 changes to direct connect, window
2's UI should update itself, right?)

[pneubeck (no reviews), 2013-04-18 07:41:59]

On 2013/04/17 23:21:20, Dan Beam wrote:
> pneubeck@: should any of the proxy settings be updated if another settings
page
> changes them?  (i.e. settings page in window 1 changes to direct connect,
window
> 2's UI should update itself, right?)

AFAIK, that never worked. See crbug/142370 which I posted a looong time ago.
This will for sure not be fixed before we revamped the UI.

[Dan Beam, 2013-04-20 00:58:46]

On 2013/04/18 07:41:59, pneubeck wrote:
> On 2013/04/17 23:21:20, Dan Beam wrote:
> > pneubeck@: should any of the proxy settings be updated if another settings
> page
> > changes them?  (i.e. settings page in window 1 changes to direct connect,
> window
> > 2's UI should update itself, right?)
> 
> AFAIK, that never worked. See crbug/142370 which I posted a looong time ago.
> This will for sure not be fixed before we revamped the UI.

in that case, lgtm

[commit-bot: I haz the power, 2013-04-20 01:01:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sathish.kuppuswamy@intel.com/13334007/...

[commit-bot: I haz the power, 2013-04-20 01:01:20]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 233.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
6c6622be0b5964eded1fea99aacf94ee27963608..a42dae7cfd833ac7c33fb08cf730f814150eb28a
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -233,3 +233,4 @@ Sam Larison <qufighter@gmail.com>
 Jun Jiang <jun.a.jiang@intel.com>
 Bobby Powers <bobbypowers@gmail.com>
 Patrick Riordan <patrickriordan177@gmail.com>
+Sathish Kuppuswamy <sathish.kuppuswamy@intel.com>

[Dan Beam, 2013-04-20 01:02:25]

On 2013/04/20 01:01:20, I haz the power (commit-bot) wrote:
> Failed to apply patch for AUTHORS:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file AUTHORS
>   Hunk #1 FAILED at 233.
>   1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej
> 
> Patch:       AUTHORS
> Index: AUTHORS
> diff --git a/AUTHORS b/AUTHORS
> index
>
6c6622be0b5964eded1fea99aacf94ee27963608..a42dae7cfd833ac7c33fb08cf730f814150eb28a
> 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -233,3 +233,4 @@ Sam Larison <mailto:qufighter@gmail.com>
>  Jun Jiang <mailto:jun.a.jiang@intel.com>
>  Bobby Powers <mailto:bobbypowers@gmail.com>
>  Patrick Riordan <mailto:patrickriordan177@gmail.com>
> +Sathish Kuppuswamy <mailto:sathish.kuppuswamy@intel.com>

you'll need to merge once more.  once you do this, upload and click:

  Commit: [X}

[Dan Beam, 2013-04-20 01:02:39]

On 2013/04/20 01:02:25, Dan Beam wrote:
> On 2013/04/20 01:01:20, I haz the power (commit-bot) wrote:
> > Failed to apply patch for AUTHORS:
> > While running patch -p1 --forward --force --no-backup-if-mismatch;
> >   patching file AUTHORS
> >   Hunk #1 FAILED at 233.
> >   1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej
> > 
> > Patch:       AUTHORS
> > Index: AUTHORS
> > diff --git a/AUTHORS b/AUTHORS
> > index
> >
>
6c6622be0b5964eded1fea99aacf94ee27963608..a42dae7cfd833ac7c33fb08cf730f814150eb28a
> > 100644
> > --- a/AUTHORS
> > +++ b/AUTHORS
> > @@ -233,3 +233,4 @@ Sam Larison <mailto:qufighter@gmail.com>
> >  Jun Jiang <mailto:jun.a.jiang@intel.com>
> >  Bobby Powers <mailto:bobbypowers@gmail.com>
> >  Patrick Riordan <mailto:patrickriordan177@gmail.com>
> > +Sathish Kuppuswamy <mailto:sathish.kuppuswamy@intel.com>
> 
> you'll need to merge once more.  once you do this, upload and click:
> 
>   Commit: [X}

  Commit: [X]**

[commit-bot: I haz the power, 2013-04-22 22:04:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sathish.kuppuswamy@intel.com/13334007/...

[commit-bot: I haz the power, 2013-04-22 23:43:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 23:52:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sathish.kuppuswamy@intel.com/13334007/...

[commit-bot: I haz the power, 2013-04-23 00:40:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-23 00:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sathish.kuppuswamy@intel.com/13334007/...

[commit-bot: I haz the power, 2013-04-23 02:16:47]

Change committed as 195690