> which uses a custom incorrect shouldEmphasizeHttpsScheme() instead of reusing the logic from the base class

Just to clarify, I meant that perhaps the logic *should* live in the base class (it doesn't currently).

That is, I think shouldEmphasizeHttpsScheme() in CustomTabToolbar.java should probably be doing the same thing as the LocationBarLayout.java implementation [1], so maybe that logic should move to the LocationBar base class.

[1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java?q=LocationBarLayout&l=1398

Actually the conditions in the base class call doesn't really apply here. We say we only emphasize when toolbar is not using any brand colors, but custom tab technically always use brand colors. (There is no way to distinguish, an app choosing the default color as their brand color vs not setting a brand color).

So the condition in CustomTabToolbar ends up emphasizing more urls, not less. We try to make sure at least dangerous ConnectionSecurityLevel is prominent.

In short, I am saying if we choose to go with the base class definition here, we will end up losing the emphasis on the ConnectionSecurityLevels listed in CustomTabToolbar as well.

Re #4: doesn't that mean that, for example, an app could set a red theme color and then a red-colored dangerous scheme wouldn't be visible?

And why can't we distinguish a brand color for CCT? Is that a fundamental limitation or is it just something that's not implemented?

Hm, also, it seems like we could just test for the default color rather than needing to know whether the app has set a brand color, couldn't we? If it's the default color, we know that scheme highlighting will show up properly, so it doesn't really matter if the app set it to the default color or it's just the default color by default.

Then the right logic becomes isUsingDefaultColor(). That would work for me. Again, we will have to have a custom tab specific logic, but that would fix the above issues. What do you think?


We can't distinguish because even if Twitter (or any other app using default color) doesn't actually call the API, we can't know whether they are not calling the API because they just prefer the default grey or they don't care. To be honest, as long as we had said isBrandColor shouldn't be emphasized, that is a statement saying we don't necessarily depend on this emphasis that much. A slightly lighter or darker gray would still not be emphasized in that setup in all toolbars. I am also OK with thinking up a better way to define when we want to use color emphasis.

> Then the right logic becomes isUsingDefaultColor(). That would work for me. Again, we will have to have a custom tab specific logic, but that would fix the above issues. What do you think?

Yeah, that seems fine to me and an improvement over what we have now. I have no objection to having CCT-specific logic if it's needed; my original suggestion that the logic should live in the base class was based on the assumption that it should be the same, which doesn't seem to be the case.

I think you're right that the coloring is not actually that important to have -- IMO it's more important that it's consistent (e.g. if we're showing a green lock, we also show a green https:// scheme) and non-clashing (e.g. we don't show a red scheme on a dark orange background). So I think we should err on the side of only coloring with isUsingDefaultToolbarColor() for now, and for a future improvement we can talk to a designer about fancier logic for when to color (cc maxwalker).

We should strive to get to a security UX bug free state by 63.

See the comment thread above for the details, but the summary is LocationBarLayout#shouldEmphasizeHttpsScheme should depend on  isUsingDefaultToolbarColor() and we should only color when we are using the default color in CCT (which is not there yet).

yusufo: Any reason why CustomTabToolbar.shouldEmphasizeHttpsScheme want to check ConnectionSecurityLevel.DANGEROUS and ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT? Looks like OmniboxUrlEmphasizer.emphasizeUrl() (https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/omnibox/OmniboxUrlEmphasizer.java?rcl=26fe601d299ed3738c440c69d81ba66782a63c7b&l=174) will change the color for ConnectionSecurityLevel.DANGEROUS and ConnectionSecurityLevel.SECURE, so ConnectionSecurityLevel.DANGEROUS is not necessary? And also it seems ConnectionSecurityLevel.SECURE_WITH_POLICY_INSTALLED_CERT is only used for Chrome OS (https://codesearch.chromium.org/chromium/src/components/security_state/core/security_state.h?rcl=71cc4cde2be4a15123b8d41786e93161cb86ad87&l=62), so overall, we don't need extra logic for CCT?

Yeah, per the discussion above, we shouldn't have any extra logic here in CCT and this condition should depend on the color value not the security state.

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

commit a1e038fe952b08e21e1fca0871dd832cfe1cb4c8
Author: Lei Tian <ltian@google.com>
Date: Tue Sep 26 21:30:06 2017

Change logic for deciding whether emphasizing the https scheme.

Both LocationBarLayout and CustomTabToolbar will emphasize the https
scheme if the toolbar uses the defualt color theme.

BUG= 726565 

Change-Id: I9d9ed97a1a245cf8262fa43008cee5184e1657ca
Reviewed-on: https://chromium-review.googlesource.com/676199
Commit-Queue: Lei Tian <ltian@google.com>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504489}
[modify] https://crrev.com/a1e038fe952b08e21e1fca0871dd832cfe1cb4c8/chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java
[modify] https://crrev.com/a1e038fe952b08e21e1fca0871dd832cfe1cb4c8/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
[modify] https://crrev.com/a1e038fe952b08e21e1fca0871dd832cfe1cb4c8/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java