ChromeOS: bypass proxy for captive portal authorization.

ChromeOS: bypass proxy for captive portal authorization.

This CL enables captive portal authorization in a separate window ignoring proxy settings.

BUG=386837
TEST=manual

Committed: https://crrev.com/94e69ad2dbb8d9cbc87154930e2a1eaf201a2c0d
Cr-Commit-Position: refs/heads/master@{#306004}

[Alexander Alekseev, 2014-11-21 22:09:15]

alemate@chromium.org changed reviewers:
+ nkostylev@chromium.org, ygorshenin@chromium.org

[Alexander Alekseev, 2014-11-21 22:09:16]

This CL is incomplete without policy to disable it.
I am going to add policy here.

But it works.

Please take a look.

[ygorshenin1, 2014-11-24 15:02:09]

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:104:
controller_->ShowDialog();
I'm proposing to add a return statement at the end to the first branch and get
rid of the else clause.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:133: :
dialog_(NULL) {
nit: s/NULL/nullptr/g

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:191: if
(!dialog_) {
nit: I'm proposing to change the code in the following way:
if (dialog_)
  return;
...

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:201: if
(dialog == dialog_) {
When dialog_ is not equal to dialog? Could you please describe such situations?
If dialog_ always equals to dialog, could you please add a CHECK_EQ at the
beginning of the method?

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.h
(right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.h:54: void
OnDialogDestroyed(const NetworkPortalWebDialog* dialog);
nit: insert a blank line between these two methods, add a comment to the
OnDialogDestroyed().

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_web_dialog.cc (right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:22: gfx::Size
display_size = display.size();  // work_area_size() ?
Could you please add a TODO comment with a bug reference?

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:80: return "";
nit: s/""/string()/g

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:91: void
NetworkPortalWebDialog::OnCloseContents(content::WebContents* source,
nit: could you please surround |source| by /* and */?

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_web_dialog.h (left):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:26: //
ui::WebDialogDelegate:
nit: could you please restore the comment?

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_web_dialog.h (right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:10: #include
"ui/views/widget/widget.h"
Could you please add a forward declaration of ui::Widget and move inclusion of
widget.h to the source file?

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:18:
NetworkPortalNotificationController* controller);
I thought it would be better if ctor will take a weak pointer as an argument.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:38:
base::WeakPtr<NetworkPortalNotificationController> controller_;
#include "base/memory/weak_ptr.h"

[Alexander Alekseev, 2014-11-26 00:26:29]

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:104:
controller_->ShowDialog();
On 2014/11/24 15:02:07, ygorshenin1 wrote:
> I'm proposing to add a return statement at the end to the first branch and get
> rid of the else clause.

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:133: :
dialog_(NULL) {
On 2014/11/24 15:02:07, ygorshenin1 wrote:
> nit: s/NULL/nullptr/g

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:191: if
(!dialog_) {
On 2014/11/24 15:02:07, ygorshenin1 wrote:
> nit: I'm proposing to change the code in the following way:
> if (dialog_)
>   return;
> ...

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:201: if
(dialog == dialog_) {
On 2014/11/24 15:02:08, ygorshenin1 wrote:
> When dialog_ is not equal to dialog? Could you please describe such
situations?
> If dialog_ always equals to dialog, could you please add a CHECK_EQ at the
> beginning of the method?

Dialog close and destroy are asynchronous. So it is possible for the first
dialog instance to be already closed, but not yet destroyed, when second one is
already instantiated.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.h
(right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.h:54: void
OnDialogDestroyed(const NetworkPortalWebDialog* dialog);
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> nit: insert a blank line between these two methods, add a comment to the
> OnDialogDestroyed().

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_web_dialog.cc (right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:22: gfx::Size
display_size = display.size();  // work_area_size() ?
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> Could you please add a TODO comment with a bug reference?

I've removed comment.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:80: return "";
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> nit: s/""/string()/g

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:91: void
NetworkPortalWebDialog::OnCloseContents(content::WebContents* source,
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> nit: could you please surround |source| by /* and */?

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_web_dialog.h (right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:10: #include
"ui/views/widget/widget.h"
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> Could you please add a forward declaration of ui::Widget and move inclusion of
> widget.h to the source file?

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:18:
NetworkPortalNotificationController* controller);
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> I thought it would be better if ctor will take a weak pointer as an argument.

Done.

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:38:
base::WeakPtr<NetworkPortalNotificationController> controller_;
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> #include "base/memory/weak_ptr.h"

Done.

[Alexander Alekseev, 2014-11-26 00:28:04]

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_web_dialog.h (left):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:26: //
ui::WebDialogDelegate:
On 2014/11/24 15:02:09, ygorshenin1 wrote:
> nit: could you please restore the comment?

Done.

[ygorshenin1, 2014-11-26 16:33:50]

lgtm

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/750153002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:104:
controller_->ShowDialog();
On 2014/11/26 00:26:28, Alexander Alekseev wrote:
> On 2014/11/24 15:02:07, ygorshenin1 wrote:
> > I'm proposing to add a return statement at the end to the first branch and
get
> > rid of the else clause.
> 
> Done.

Sorry, I missed CloseNotification() call. Could you please restore your code for
the function back?

https://codereview.chromium.org/750153002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_web_dialog.cc (right):

https://codereview.chromium.org/750153002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:43: :
controller_(controller), widget_(NULL) {
nit: s/NULL/nullptr/g

[Alexander Alekseev, 2014-11-26 17:51:00]

https://codereview.chromium.org/750153002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_web_dialog.cc (right):

https://codereview.chromium.org/750153002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:43: :
controller_(controller), widget_(NULL) {
On 2014/11/26 16:33:50, ygorshenin1 wrote:
> nit: s/NULL/nullptr/g

Done.

[Nikita (slow), 2014-11-26 18:02:57]

lgtm with nits

https://codereview.chromium.org/750153002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/750153002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:14075: +          If proxy is configured, it
usually prevents from authorization on different captive portals. This option
enables opening captive portal authorization dialog in a separate window, which
ignores proxy settings.
nit: This flag enables

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:199:
chrome::ShowWebDialog(NULL, signin_profile, dialog_)));
nit: nullptr

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:205:
dialog_ = NULL;
nit: nullptr

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_web_dialog.cc (right):

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:65: return
base::string16();
nit: I guess that this should contain a real string here once this is enabled by
default.

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_web_dialog.h (right):

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:19: class
NetworkPortalWebDialog : public ui::WebDialogDelegate {
nit: Add class comment i.e. what it is, where it is used.

[Alexander Alekseev, 2014-11-26 19:42:42]

https://codereview.chromium.org/750153002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/750153002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:14075: +          If proxy is configured, it
usually prevents from authorization on different captive portals. This option
enables opening captive portal authorization dialog in a separate window, which
ignores proxy settings.
On 2014/11/26 18:02:57, Nikita (OOO Nov 29 - Jan 11) wrote:
> nit: This flag enables

Done.

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_notification_controller.cc
(right):

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:199:
chrome::ShowWebDialog(NULL, signin_profile, dialog_)));
On 2014/11/26 18:02:57, Nikita (OOO Nov 29 - Jan 11) wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_notification_controller.cc:205:
dialog_ = NULL;
On 2014/11/26 18:02:57, Nikita (OOO Nov 29 - Jan 11) wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_web_dialog.cc (right):

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_web_dialog.cc:65: return
base::string16();
On 2014/11/26 18:02:57, Nikita (OOO Nov 29 - Jan 11) wrote:
> nit: I guess that this should contain a real string here once this is enabled
by
> default.

Done.

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/network_portal_web_dialog.h (right):

https://codereview.chromium.org/750153002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/network_portal_web_dialog.h:19: class
NetworkPortalWebDialog : public ui::WebDialogDelegate {
On 2014/11/26 18:02:57, Nikita (OOO Nov 29 - Jan 11) wrote:
> nit: Add class comment i.e. what it is, where it is used.

Done.

[Alexander Alekseev, 2014-11-26 19:44:41]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-11-26 19:45:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750153002/120001

[commit-bot: I haz the power, 2014-11-26 20:52:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-26 20:52:04]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Alekseev, 2014-11-27 14:30:23]

alemate@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexander Alekseev, 2014-11-27 14:30:24]

asvitkine@ please review histograms.xml.

[Alexei Svitkine (slow), 2014-11-27 15:57:47]

histograms lgtm

[Alexander Alekseev, 2014-11-27 16:30:53]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-11-27 16:31:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/750153002/160001

[commit-bot: I haz the power, 2014-11-27 17:30:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-11-27 17:31:22]

Patchset 9 (id:??) landed as
https://crrev.com/94e69ad2dbb8d9cbc87154930e2a1eaf201a2c0d
Cr-Commit-Position: refs/heads/master@{#306004}