Show a custom page info bubble for chrome-devtools:// URLs

Show a custom page info bubble for chrome-devtools:// URLs

- Use the "you are viewing a secure Chrome page" bubble for devtools URLs.
- Add interactive UI tests.
- Remove unit tests for unreached code.

BUG=653171
Committed: https://crrev.com/de53fcff417a217815f0b0830cbf6265ba1f75ae
Cr-Commit-Position: refs/heads/master@{#424817}

[meacer, 2016-10-06 19:41:43]

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

[commit-bot: I haz the power, 2016-10-06 19:42:12]

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-10-06 20:11:04]

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

[commit-bot: I haz the power, 2016-10-06 20:11:05]

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_...)

[meacer, 2016-10-06 20:26:37]

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

[commit-bot: I haz the power, 2016-10-06 20:27:02]

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-10-06 21:00:17]

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

[commit-bot: I haz the power, 2016-10-06 21:00:18]

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_...)

[meacer, 2016-10-06 23:14:40]

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

[commit-bot: I haz the power, 2016-10-06 23:15:27]

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

[meacer, 2016-10-06 23:25:00]

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

[commit-bot: I haz the power, 2016-10-06 23:25:22]

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-10-06 23:43:19]

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

[commit-bot: I haz the power, 2016-10-06 23:43:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meacer, 2016-10-06 23:50:24]

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

[commit-bot: I haz the power, 2016-10-06 23:50:52]

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-10-07 00:40:37]

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

[commit-bot: I haz the power, 2016-10-07 00:40:37]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-10-07 20:01:19]

Description was changed from

==========
Show a custom page info bubble for chrome-devtools:// URLs

BUG=653171
==========

to

==========
Show a custom page info bubble for chrome-devtools:// URLs

- Use the "you are viewing a secure Chrome page" bubble for devtools URLs.
- Add interactive UI tests.
- Remove unit tests for unreached code.

BUG=653171
==========

[meacer, 2016-10-07 20:03:04]

meacer@chromium.org changed reviewers:
+ elawrence@chromium.org

[meacer, 2016-10-07 20:03:05]

elawrence: PTAL? This shows a custom popup for devtools URLs but doesn't fix the
non-secure strikethrough yet.

[elawrence, 2016-10-07 21:04:01]

Looks good to me! I had a few questions owing to my unfamiliarity with this
area.

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(left):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:441:
bool WebsiteSettingsPopupView::IsPopupShowing() {
Given the calling patterns, does it make sense to keep the IsPopupShowing()
method around and just change it to check whether shown_popup_type ==
POPUP_NONE? It seems like a lot of callers won't care about the specifics, only
whether *any* popup is showing.

Or should we not do that, because now we set shown_popup_type in the constructor
and don't wait for ShowPopup to actually get called?

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/website_settings/website_settings_popup_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view_interactive_uitest.cc:65:
ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
Is there a reason we can't go directly to "view-source:about:blank" here?

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:403: // On desktop,
internal URLs are displayed in a different UI and not here.
Is "desktop" mutually exclusive with "ANDROID_JAVA_UI" (e.g. what about bling?)

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:423: if
(isChromeUINativeScheme) {
I'm not sure I understand this change. Is this:

    url.SchemeIs(content::kChromeUIScheme) 

... not possible when ANDROID_JAVA_UI?

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (left):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_unittest.cc:870:
SetURL("chrome://bookmarks");
Was this test removed because it's obsoleted by the chrome://settings case
inside website_settings_popup_view_interactive_uitest.cc?

[meacer, 2016-10-07 21:58:48]

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

[commit-bot: I haz the power, 2016-10-07 21:59:15]

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-10-07 22:36:04]

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

[commit-bot: I haz the power, 2016-10-07 22:36:05]

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_...)

[meacer, 2016-10-08 00:42:39]

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

[commit-bot: I haz the power, 2016-10-08 00:42:56]

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-10-08 01:44:19]

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

[commit-bot: I haz the power, 2016-10-08 01:44:20]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-10-10 18:09:19]

Thanks Eric. PTAL?

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(left):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:441:
bool WebsiteSettingsPopupView::IsPopupShowing() {
On 2016/10/07 21:04:01, elawrence wrote:
> Given the calling patterns, does it make sense to keep the IsPopupShowing()
> method around and just change it to check whether shown_popup_type ==
> POPUP_NONE? It seems like a lot of callers won't care about the specifics,
only
> whether *any* popup is showing.

I thought about doing that, but it seemed a bit redundant to have two methods to
check what's essentially the same thing.

I don't have a strong opinion though, I could go either way here. 

> 
> Or should we not do that, because now we set shown_popup_type in the
constructor
> and don't wait for ShowPopup to actually get called?

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
File
chrome/browser/ui/views/website_settings/website_settings_popup_view_interactive_uitest.cc
(right):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/website_settings_popup_view_interactive_uitest.cc:65:
ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
On 2016/10/07 21:04:01, elawrence wrote:
> Is there a reason we can't go directly to "view-source:about:blank" here?

Yes, there seems to be a bug in handling view-source URLs in test utils.
Navigating to view-source:about:blank ends up at about:blank for some reason
that I was too lazy to investigate for this CL :)

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:403: // On desktop,
internal URLs are displayed in a different UI and not here.
On 2016/10/07 21:04:01, elawrence wrote:
> Is "desktop" mutually exclusive with "ANDROID_JAVA_UI" (e.g. what about
bling?)

Ah, ANDROID_JAVA_UI really confused me. I thought it was default on Android
builds, but apparently it's not and it's going to be removed soon. I moved the
check to above.

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings.cc:423: if
(isChromeUINativeScheme) {
On 2016/10/07 21:04:01, elawrence wrote:
> I'm not sure I understand this change. Is this:
> 
>     url.SchemeIs(content::kChromeUIScheme) 
> 
> ... not possible when ANDROID_JAVA_UI?

I was being overly aggressive here, reverted this change.

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (left):

https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_unittest.cc:870:
SetURL("chrome://bookmarks");
On 2016/10/07 21:04:01, elawrence wrote:
> Was this test removed because it's obsoleted by the chrome://settings case
> inside website_settings_popup_view_interactive_uitest.cc? 

I removed it because I thought it would never be possible for
website_settings.cc to display a chrome:// URL. That's not true on Android, so
reverting this change too.

[elawrence, 2016-10-10 18:21:21]

On 2016/10/10 18:09:19, Mustafa Emre Acer wrote:
> Thanks Eric. PTAL?
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
> File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
> (left):
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
> chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:441:
> bool WebsiteSettingsPopupView::IsPopupShowing() {
> On 2016/10/07 21:04:01, elawrence wrote:
> > Given the calling patterns, does it make sense to keep the IsPopupShowing()
> > method around and just change it to check whether shown_popup_type ==
> > POPUP_NONE? It seems like a lot of callers won't care about the specifics,
> only
> > whether *any* popup is showing.
> 
> I thought about doing that, but it seemed a bit redundant to have two methods
to
> check what's essentially the same thing.
> 
> I don't have a strong opinion though, I could go either way here. 
> 
> > 
> > Or should we not do that, because now we set shown_popup_type in the
> constructor
> > and don't wait for ShowPopup to actually get called?
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
> File
>
chrome/browser/ui/views/website_settings/website_settings_popup_view_interactive_uitest.cc
> (right):
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/views...
>
chrome/browser/ui/views/website_settings/website_settings_popup_view_interactive_uitest.cc:65:
> ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
> On 2016/10/07 21:04:01, elawrence wrote:
> > Is there a reason we can't go directly to "view-source:about:blank" here?
> 
> Yes, there seems to be a bug in handling view-source URLs in test utils.
> Navigating to view-source:about:blank ends up at about:blank for some reason
> that I was too lazy to investigate for this CL :)
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings.cc (right):
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings.cc:403: // On desktop,
> internal URLs are displayed in a different UI and not here.
> On 2016/10/07 21:04:01, elawrence wrote:
> > Is "desktop" mutually exclusive with "ANDROID_JAVA_UI" (e.g. what about
> bling?)
> 
> Ah, ANDROID_JAVA_UI really confused me. I thought it was default on Android
> builds, but apparently it's not and it's going to be removed soon. I moved the
> check to above.
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings.cc:423: if
> (isChromeUINativeScheme) {
> On 2016/10/07 21:04:01, elawrence wrote:
> > I'm not sure I understand this change. Is this:
> > 
> >     url.SchemeIs(content::kChromeUIScheme) 
> > 
> > ... not possible when ANDROID_JAVA_UI?
> 
> I was being overly aggressive here, reverted this change.
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
> File chrome/browser/ui/website_settings/website_settings_unittest.cc (left):
> 
>
https://codereview.chromium.org/2397273002/diff/80001/chrome/browser/ui/websi...
> chrome/browser/ui/website_settings/website_settings_unittest.cc:870:
> SetURL("chrome://bookmarks");
> On 2016/10/07 21:04:01, elawrence wrote:
> > Was this test removed because it's obsoleted by the chrome://settings case
> > inside website_settings_popup_view_interactive_uitest.cc? 
> 
> I removed it because I thought it would never be possible for
> website_settings.cc to display a chrome:// URL. That's not true on Android, so
> reverting this change too.

(Provisional-committer)LGTM

[meacer, 2016-10-10 18:33:47]

Thanks.

Owners, can you please take a look at the following?

pkasting@ for chrome/browser/ui/views/location_bar/
rsesek@ for chrome/browser/ui/cocoa/website_settings/
raymes@ for chrome/browser/ui/views/website_settings/ and
chrome/browser/ui/website_settings/

[meacer, 2016-10-10 18:34:26]

meacer@chromium.org changed reviewers:
+ pkasting@chromium.org, raymes@chromium.org, rsesek@chromium.org

[meacer, 2016-10-10 18:34:27]

Adding again:

pkasting@ for chrome/browser/ui/views/location_bar/
rsesek@ for chrome/browser/ui/cocoa/website_settings/
raymes@ for chrome/browser/ui/views/website_settings/ and
chrome/browser/ui/website_settings/

[Peter Kasting, 2016-10-11 00:44:14]

LGTM

[Robert Sesek, 2016-10-11 14:20:33]

lgtm

[raymes, 2016-10-12 02:43:12]

lgtm

https://codereview.chromium.org/2397273002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2397273002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:73:
WebsiteSettingsPopupView::PopupType shown_popup_type =
nit: I think globals tend to be prefixed with g_ (it's a little scary that this
is a global, but I know you're not really changing that :)

[meacer, 2016-10-12 17:47:51]

Patchset #9 (id:160001) has been deleted

[meacer, 2016-10-12 17:48:39]

Thanks all.

https://codereview.chromium.org/2397273002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2397273002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:73:
WebsiteSettingsPopupView::PopupType shown_popup_type =
On 2016/10/12 02:43:12, raymes wrote:
> nit: I think globals tend to be prefixed with g_ (it's a little scary that
this
> is a global, but I know you're not really changing that :)

Renamed, and sad global is sad :)

[meacer, 2016-10-12 17:48:43]

The CQ bit was checked by meacer@chromium.org

[meacer, 2016-10-12 17:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
elawrence@chromium.org, pkasting@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2397273002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-12 17:49:13]

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-10-12 19:22:30]

Description was changed from

==========
Show a custom page info bubble for chrome-devtools:// URLs

- Use the "you are viewing a secure Chrome page" bubble for devtools URLs.
- Add interactive UI tests.
- Remove unit tests for unreached code.

BUG=653171
==========

to

==========
Show a custom page info bubble for chrome-devtools:// URLs

- Use the "you are viewing a secure Chrome page" bubble for devtools URLs.
- Add interactive UI tests.
- Remove unit tests for unreached code.

BUG=653171
==========

[commit-bot: I haz the power, 2016-10-12 19:22:31]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-10-12 19:24:05]

Description was changed from

==========
Show a custom page info bubble for chrome-devtools:// URLs

- Use the "you are viewing a secure Chrome page" bubble for devtools URLs.
- Add interactive UI tests.
- Remove unit tests for unreached code.

BUG=653171
==========

to

==========
Show a custom page info bubble for chrome-devtools:// URLs

- Use the "you are viewing a secure Chrome page" bubble for devtools URLs.
- Add interactive UI tests.
- Remove unit tests for unreached code.

BUG=653171

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

[commit-bot: I haz the power, 2016-10-12 19:24:06]

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