Leave the '://' grey in android's page info dialog.

Leave the '://' grey in android's page info dialog.

Previously it was colored black with the origin.

BUG=430771
Committed: https://crrev.com/bd0521db1aa40c0f72177c171896069c71ea2327
Cr-Commit-Position: refs/heads/master@{#303976}

[benwells, 2014-11-07 06:13:43]

benwells@chromium.org changed reviewers:
+ tedchoc@chromium.org

[benwells, 2014-11-07 06:13:44]

tedchoc: i've never done java, so i probably got it all wrong :)

[benwells, 2014-11-09 20:52:51]

On 2014/11/07 06:13:44, benwells wrote:
> tedchoc: i've never done java, so i probably got it all wrong :)

ping tedchoc..

[Ted C, 2014-11-10 19:48:15]

https://codereview.chromium.org/706113006/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/706113006/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:305:
String originToDisplay = UrlUtilities.getOriginForDisplay(parsedUrl, false);
What does this return for about:blank?

I think the host will be empty as URI doesn't understand the : without //
syntax, so this call will probably return about:blank as the entire string if I
recall.

So I think the originBegin will be -1 below.

Maybe the check below needs to be wrapped in condition that the originBegin >= 0

[benwells, 2014-11-11 03:21:44]

Patchset #2 (id:20001) has been deleted

[benwells, 2014-11-11 03:28:20]

https://codereview.chromium.org/706113006/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/706113006/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:305:
String originToDisplay = UrlUtilities.getOriginForDisplay(parsedUrl, false);
On 2014/11/10 19:48:14, Ted C wrote:
> What does this return for about:blank?
> 
> I think the host will be empty as URI doesn't understand the : without //
> syntax, so this call will probably return about:blank as the entire string if
I
> recall.
> 
> So I think the originBegin will be -1 below.
> 
> Maybe the check below needs to be wrapped in condition that the originBegin >=
0

Oh dear, good catch.

I've updated it to handle this case.

[Ted C, 2014-11-11 23:10:38]

lgtm w/ a style nit (feel free to ignore the other question)

https://codereview.chromium.org/706113006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/706113006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:317:
if (originBegin < parsedUrlString.length())
in java land, braces are always required if the statement and conditional do not
fit on the same line.

but in this case, it might all fit on one line?

https://codereview.chromium.org/706113006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:318:
originBegin++;
This really only works on about:blank

If for some reason the parsing fails on a :// scheme, then the // will still be
incorrectly tinted.

I don't know if it's worth checking both cases, but just something to consider.

[benwells, 2014-11-13 02:32:22]

https://codereview.chromium.org/706113006/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/706113006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:317:
if (originBegin < parsedUrlString.length())
On 2014/11/11 23:10:38, Ted C wrote:
> in java land, braces are always required if the statement and conditional do
not
> fit on the same line.
> 
> but in this case, it might all fit on one line?

Indeed it does.

https://codereview.chromium.org/706113006/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:318:
originBegin++;
On 2014/11/11 23:10:38, Ted C wrote:
> This really only works on about:blank
> 
> If for some reason the parsing fails on a :// scheme, then the // will still
be
> incorrectly tinted.
> 
> I don't know if it's worth checking both cases, but just something to
consider.

When I looked at the java URI.getHost documentation I couldn't think of any
other cases apart from scheme:foo links that would be accepted by chrome but
wouldn't return the host. Do you have any examples in mind?

[benwells, 2014-11-13 02:32:25]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2014-11-13 02:33:25]

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

[commit-bot: I haz the power, 2014-11-13 03:27:03]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-11-13 03:28:58]

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