I think matching the Clank scheme display for Offline pages is important. Offline pages are not the real content delivered by the server and in some cases the source of the offline page might not be clear; it could come from somewhere entirely untrusted.

I would not necessarily block the security bit flip on this alone, but I think it's very important and a must-have for 62 if it doesn't make it into 61.

Assigning to Tommy for guidance on how to best use kFormatUrlExperimentalOmitHTTPS in non-experimental code.  Tommy, if you can comment and bounce this back to me, that'd be great.

A bit more context:  Chrome VR on Android draws its own UI - it does not use Android's UI, or Views-based Desktop UI, at this time.  Hence, we are trying to keep code as close to desktop as possible, while still handling Android-isms (like offline mode).

If you need more details, feel free to IM, email or ask here.  Thanks!

Hey,

So as far as I'm concerned, that flag is fine to use (in terms of stability, correctness, and test coverage).

The reason it's marked "experimental" is that it was only planned to be used for a UI experiment. If you want it to be used "for reals", here is my recommendation on what you should do:

 1. Rename it to remove "experimental" from the name.

 2. Figure out what kFormatUrlOmitAll now means. I suspect it really means "default" or "sane defaults" and should be renamed kFormatUrlOmitDefaults or something. This may take a bit of investigation into the callsites.

 3. Re-jigger the comments to make clear that the flags within the default set don't include the experimental ones and the HTTPS trimming.

Thanks,

Tommy

Thanks Tommy!  Your recommendations make total sense.  Taking back the bug.

Enamel (and I) would like to fix this for M-61, so my plan is to first fix our bug using the existing experimental-named constant, merge the fix back (touching VR code only), then do the refactor you recommended.  That'll give us the correct solution without putting extra risk on the branch.

cjgrant: What you just described sounds good to me.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52699acc5b78f5fd5c3be69f7051dd2ab669f1f3

commit 52699acc5b78f5fd5c3be69f7051dd2ab669f1f3
Author: Christopher Grant <cjgrant@chromium.org>
Date: Tue Jul 25 21:16:38 2017

VR: Strip https scheme from offline pages.

As per Clank behavior and Enamel's request, offline pages should not
show an https scheme.

BUG= 740112 

Change-Id: I46bc34115d72ae83b2f76874279d125152d69ac7
Reviewed-on: https://chromium-review.googlesource.com/585305
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489435}
[modify] https://crrev.com/52699acc5b78f5fd5c3be69f7051dd2ab669f1f3/chrome/browser/vr/elements/url_bar_texture.cc
[modify] https://crrev.com/52699acc5b78f5fd5c3be69f7051dd2ab669f1f3/chrome/browser/vr/elements/url_bar_texture.h
[modify] https://crrev.com/52699acc5b78f5fd5c3be69f7051dd2ab669f1f3/chrome/browser/vr/elements/url_bar_texture_unittest.cc

Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b58a4ead370d9fe6a49cdae2b0cb18fb1db22305

commit b58a4ead370d9fe6a49cdae2b0cb18fb1db22305
Author: Christopher Grant <cjgrant@chromium.org>
Date: Thu Jul 27 14:19:06 2017

VR: Strip https scheme from offline pages.

As per Clank behavior and Enamel's request, offline pages should not
show an https scheme.

BUG= 740112 

Change-Id: I46bc34115d72ae83b2f76874279d125152d69ac7
Reviewed-on: https://chromium-review.googlesource.com/585305
Reviewed-by: Ian Vollick <vollick@chromium.org>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489435}(cherry picked from commit 52699acc5b78f5fd5c3be69f7051dd2ab669f1f3)
Reviewed-on: https://chromium-review.googlesource.com/589487
Reviewed-by: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#79}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/b58a4ead370d9fe6a49cdae2b0cb18fb1db22305/chrome/browser/vr/elements/url_bar_texture.cc
[modify] https://crrev.com/b58a4ead370d9fe6a49cdae2b0cb18fb1db22305/chrome/browser/vr/elements/url_bar_texture.h
[modify] https://crrev.com/b58a4ead370d9fe6a49cdae2b0cb18fb1db22305/chrome/browser/vr/elements/url_bar_texture_unittest.cc

 Issue 749526  has been filed to track the renaming work Tommy recommended.

Hi, I'm trying to verify this. Could you describe what the https scheme is? I've attached a screenshot of the current url bar when viewing offline content. Is it correct? Thanks! Are there other parts of the UI to verify?

Using Chrome 61.0.3163.13

Deleted: offline_url_bar.png 18.7 KB

	offline_url_bar.png 18.7 KB View Download

The image you included hasn't picked up the fix.  When it makes it into your build, you should no longer see "https://" in front of the URL.

The image you've pasted should change to look like:

[icon] Offline | mobile.nytimes.com/?ref...

The background here is that if you're viewing offline content, there isn't an https connection supplying it, despite the fact that it might have originally come from an https connection (I believe).

Verified with latest M65 build.