Http Bad: Add "Learn more" sublabel to warning message and make it clickable

Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298, 662297
Committed: https://crrev.com/b0cb5e4ea319515a8de9c9e9720fd9a79beeb882
Cr-Commit-Position: refs/heads/master@{#435224}

[lshang, 2016-11-15 05:51:09]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 05:51:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 05:58:21]

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

[commit-bot: I haz the power, 2016-11-15 05:58:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lshang, 2016-11-15 07:17:54]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[lshang, 2016-11-15 07:17:59]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-15 07:18:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-11-15 07:18:06]

Patchset #1 (id:1) has been deleted

[lshang, 2016-11-15 07:27:30]

Description was changed from

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable


Merge branch 'style_http_warning_message' into http_bad_make_clickable


Merge branch 'style_http_warning_message' into http_bad_make_clickable


format


some change

BUG=
==========

to

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298. 662297
==========

[lshang, 2016-11-15 07:28:18]

Description was changed from

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298. 662297
==========

to

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298, 662297
==========

[lshang, 2016-11-15 07:28:30]

Patchset #1 (id:20001) has been deleted

[lshang, 2016-11-15 07:29:36]

lshang@chromium.org changed reviewers:
+ mathp@chromium.org

[lshang, 2016-11-15 07:29:36]

PTAL thanks!

[commit-bot: I haz the power, 2016-11-15 07:48:42]

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

[commit-bot: I haz the power, 2016-11-15 07:48:43]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lshang, 2016-11-15 08:09:24]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 08:09:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 09:30:18]

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

[commit-bot: I haz the power, 2016-11-15 09:30:19]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-11-16 05:14:11]

On 2016/11/15 07:29:36, lshang wrote:
> PTAL thanks!

There is also
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/autofill/autofill_...

Can we have tests similar to how we test the signin promo opens the signin
dialog?

Thanks

[lshang, 2016-11-16 11:09:52]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 11:10:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-11-16 11:10:21]

Patchset #1 (id:60001) has been deleted

[lshang, 2016-11-16 11:12:18]

Thanks Mathieu! PTAL again?

On 2016/11/16 05:14:11, Mathieu Perreault wrote:
> On 2016/11/15 07:29:36, lshang wrote:
> > PTAL thanks!
> 
> There is also
>
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/autofill/autofill_...
> 

Done. Hmm...I wonder why trybots didn't complain about this.

> Can we have tests similar to how we test the signin promo opens the signin
> dialog?
> 
> Thanks

Tests added.

[commit-bot: I haz the power, 2016-11-16 13:43:14]

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

[commit-bot: I haz the power, 2016-11-16 13:43:14]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-11-16 20:03:46]

Hi I'm so sorry I had this draft comment saved and it didn't send!

I think we can skip the autofill_manager plumbing.

https://codereview.chromium.org/2505533002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2505533002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:232:
manager_->OpenUrl(GURL(kSecurityIndicatorHelpCenterURL));
use manager_->client()->OpenUrl directly?

[lshang, 2016-11-17 07:13:13]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 07:13:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-11-17 07:17:23]

https://codereview.chromium.org/2505533002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2505533002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:232:
manager_->OpenUrl(GURL(kSecurityIndicatorHelpCenterURL));
On 2016/11/16 20:03:46, Mathieu Perreault wrote:
> use manager_->client()->OpenUrl directly?

Done.

[commit-bot: I haz the power, 2016-11-17 09:51:13]

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

[commit-bot: I haz the power, 2016-11-17 09:51:14]

Dry run: This issue passed the CQ dry run.

[lshang, 2016-11-20 09:17:04]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-20 09:17:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-20 13:18:14]

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

[commit-bot: I haz the power, 2016-11-20 13:18:15]

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

[Mathieu, 2016-11-22 14:40:13]

mathp@chromium.org changed reviewers:
+ estade@chromium.org

[Mathieu, 2016-11-22 14:40:14]

lgtm, sorry again for the delay, I was away.

estade@chromium.org: Please review changes in 

ui/autofill

[Evan Stade, 2016-11-22 16:36:28]

https://codereview.chromium.org/2505533002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2505533002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:33: const char
kSecurityIndicatorHelpCenterURL[] =
nit: kSecurityIndicatorHelpCenterUrl

https://codereview.chromium.org/2505533002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:232:
manager_->client()->OpenUrl(GURL(kSecurityIndicatorHelpCenterURL));
It sorta seems like an implementation detail of ChromeAutofillClient that the
warning message is displayed by opening a tab to a specific URL. In other words,
I might suggest making the new function "ShowHttpNotSecureExplanation()" or
something.

Compare to ShowAutofillSettings(), which does different things on Android and
desktop. Even though we might want to do the same thing currently for both (i.e.
open a new tab that goes to a particular webpage) that might not always be true.
Philosophically I think the ChromeAutofillClient should be in charge of what UI
should be displayed and specifying a URL in this file is a small layering
violation.

[lshang, 2016-11-23 11:07:19]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 11:07:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 11:17:42]

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

[commit-bot: I haz the power, 2016-11-23 11:17:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[lshang, 2016-11-23 11:24:19]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 11:24:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lshang, 2016-11-23 11:24:44]

Patchset #5 (id:160001) has been deleted

[lshang, 2016-11-23 11:24:56]

Patchset #5 (id:180001) has been deleted

[lshang, 2016-11-23 11:26:20]

Thanks Mathieu and Evan! PTAL again?

https://codereview.chromium.org/2505533002/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2505533002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:33: const char
kSecurityIndicatorHelpCenterURL[] =
On 2016/11/22 16:36:28, Evan Stade wrote:
> nit: kSecurityIndicatorHelpCenterUrl

Done.

https://codereview.chromium.org/2505533002/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_external_delegate.cc:232:
manager_->client()->OpenUrl(GURL(kSecurityIndicatorHelpCenterURL));
On 2016/11/22 16:36:28, Evan Stade wrote:
> It sorta seems like an implementation detail of ChromeAutofillClient that the
> warning message is displayed by opening a tab to a specific URL. In other
words,
> I might suggest making the new function "ShowHttpNotSecureExplanation()" or
> something.
> 
> Compare to ShowAutofillSettings(), which does different things on Android and
> desktop. Even though we might want to do the same thing currently for both
(i.e.
> open a new tab that goes to a particular webpage) that might not always be
true.
> Philosophically I think the ChromeAutofillClient should be in charge of what
UI
> should be displayed and specifying a URL in this file is a small layering
> violation.

hmm...I called it OpenUrl() since I thought it might be reused in the future,
but yeah your consideration makes more sense at the moment.

Done.

[commit-bot: I haz the power, 2016-11-23 12:34:37]

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

[commit-bot: I haz the power, 2016-11-23 12:34:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2016-11-23 16:43:29]

mathp@chromium.org changed reviewers:
+ blundell@chromium.org, torne@chromium.org, vabr@chromium.org

[Mathieu, 2016-11-23 16:43:30]

torne@chromium.org: Please review changes in 
webview

vabr@chromium.org: Please review changes in 
password_manager

blundell@chromium.org: Please review changes in 
ios/

[Mathieu, 2016-11-23 16:44:49]

mathp@chromium.org changed reviewers:
+ sdefresne@chromium.org
- blundell@chromium.org

[Mathieu, 2016-11-23 16:45:09]

Sorry -blundell +sdefresne, please review ios/

[vabr (Chromium), 2016-11-23 17:33:37]

password_manager LGTM, thanks!

[Torne, 2016-11-23 17:54:22]

android_webview LGTM

[sdefresne, 2016-11-24 10:28:33]

ios/ lgtm

[Evan Stade, 2016-11-28 16:19:55]

lgtm

[lshang, 2016-11-29 23:59:44]

The CQ bit was checked by lshang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 00:00:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 02:04:07]

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

[commit-bot: I haz the power, 2016-11-30 02:04:08]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[lshang, 2016-11-30 09:16:11]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-11-30 09:16:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
vabr@chromium.org, sdefresne@chromium.org, torne@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2505533002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-30 09:16:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 09:19:55]

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

[commit-bot: I haz the power, 2016-11-30 09:19:57]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lshang, 2016-11-30 10:47:44]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-11-30 10:47:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
vabr@chromium.org, sdefresne@chromium.org, torne@chromium.org,
estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2505533002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-30 10:48:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 11:53:53]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480502864427240,
"parent_rev": "175476ee5718ccbc682c825773f1f606f969744d", "commit_rev":
"60cd20df19616911d5783846c08c7b2b8153c34f"}

[commit-bot: I haz the power, 2016-11-30 11:54:22]

Description was changed from

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298, 662297
==========

to

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298, 662297
==========

[commit-bot: I haz the power, 2016-11-30 11:54:24]

Committed patchset #7 (id:240001)

[commit-bot: I haz the power, 2016-11-30 11:58:35]

Description was changed from

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298, 662297
==========

to

==========
Http Bad: Add "Learn more" sublabel to warning message and make it clickable

This CL adds a "Learn more" sublabel to the http bad warning message, and makes
the dropdown item to be clickable and redirect to the security indicator help
center page.

BUG=662298, 662297

Committed: https://crrev.com/b0cb5e4ea319515a8de9c9e9720fd9a79beeb882
Cr-Commit-Position: refs/heads/master@{#435224}
==========

[commit-bot: I haz the power, 2016-11-30 11:58:37]

Patchset 7 (id:??) landed as
https://crrev.com/b0cb5e4ea319515a8de9c9e9720fd9a79beeb882
Cr-Commit-Position: refs/heads/master@{#435224}