Update scheme text color in Omnibox only in non-default Security Levels

Update scheme text color in Omnibox only in non-default Security Levels

A recent refactoring dropped a test from the conditional which caused
the Omnibox to paint the scheme text in black in cases where it should
not. Restore the conditional so that the Omnibox only colors the scheme
text in the correct scenarios.

BUG=699222
TEST=Type about:blank in the omnibox and hit enter. "about:" should be
grey and "blank" should be black

Review-Url: https://codereview.chromium.org/2734783007
Cr-Commit-Position: refs/heads/master@{#455783}
Committed: https://chromium.googlesource.com/chromium/src/+/f079e7398cefd26400234c38a83630d2f5730969

[elawrence, 2017-03-07 20:22:02]

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

[elawrence, 2017-03-07 20:22:02]

PTAL?

[elawrence, 2017-03-07 21:53:35]

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

[commit-bot: I haz the power, 2017-03-07 21:54:36]

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, 2017-03-07 22:51:24]

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

[commit-bot: I haz the power, 2017-03-07 22:51:25]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-03-08 03:05:44]

Seems like the test suite should be expanded to catch this (though maybe you're
waiting for my in-progress changes there to land?).

https://codereview.chromium.org/2734783007/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2734783007/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:577: // Scheme color is
only overridden for non-default security levels.
Mac also does this check.  Maybe we should pass the security level to
UpdateTextStyle() so it can do this.

It would also be good to explain this better.  This comment talks about "what",
which I can kind of gather from the code, but it doesn't talk about "why", which
honestly is a bit unclear to me as well.

[elawrence, 2017-03-08 17:23:03]

Thanks for the quick feedback!

> Seems like the test suite should be expanded to catch 
> this (though maybe you're waiting for my in-progress
> changes there to land?).

I need to port this tweak to M58, so changing the new tests would require
porting your CLs there as well. 

However, I don't think adding new tests to OmniboxViewViewsTest.Emphasis would
help, as the entire refactoring that led to that test was embarked upon to deal
with the fact that we don't have a way to write the kind of test that I
originally wanted to write-- one that looks at the styling in the Omnibox's
textfield. To address that, we settled upon the virtual function approach
described in https://codereview.chromium.org/2641003002#msg19 that allows us to
validate what virtual functions are called by the emphasizer... but not what
those overridden functions actually *do*.

I /think/ the way to actually get the raw styling would entail making
RenderTextTestApi a class consumable by other tests, then add a new
IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, URLEmphasisTests) that does
something like

  gfx::RenderText* render_text = omnibox_view_views->GetRenderText();
  RenderTextTestApi text_api = RenderTextTestApi(render_text);

... and then uses the RenderTextTestApi to walk over the ranges of text and
validates that each is as expected. While that might be worthwhile in the
long-term, I'd like to get this immediate regression fix landed ASAP.

https://codereview.chromium.org/2734783007/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2734783007/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:577: // Scheme color is
only overridden for non-default security levels.
On 2017/03/08 03:05:44, Peter Kasting wrote:
> Mac also does this check.  Maybe we should pass the security level to
> UpdateTextStyle() so it can do this.
> 
> It would also be good to explain this better.  This comment talks about
"what",
> which I can kind of gather from the code, but it doesn't talk about "why",
which
> honestly is a bit unclear to me as well.

I've updated the comment. 

Passing the security level up to UpdateTextStyle would require a significant
refactoring because we currently get the level in different ways on Mac vs.
Views, and we'd either need to pass it back down to UpdateSchemeStyle or create
a new set of flag arguments to UpdateSchemeStyle that explain exactly what we
want to have done (StrikeThrough|WarnColor, PlainText|EVColor) etc. 

I feel pretty good about the current code; it's much simpler than it was prior
to this effort and now iOS/Mac/Views are all very similar, with small functions
implementing the platform specific pieces.

[Peter Kasting, 2017-03-09 06:02:42]

LGTM.  If you're confident about the behavior change question I have below, go
ahead and land (but do explain to me so I can understand as well).  If you're
not confident, then wait for me to parse your explanation :)

On 2017/03/08 17:23:03, elawrence wrote:
> However, I don't think adding new tests to OmniboxViewViewsTest.Emphasis would
> help, as the entire refactoring that led to that test was embarked upon to
deal
> with the fact that we don't have a way to write the kind of test that I
> originally wanted to write-- one that looks at the styling in the Omnibox's
> textfield. To address that, we settled upon the virtual function approach
> described in https://codereview.chromium.org/2641003002#msg19 that allows us
to
> validate what virtual functions are called by the emphasizer... but not what
> those overridden functions actually *do*.

FWIW, this is connected to my comment about doing the security state check in
the shared code; this would make it possible for the test to see that the method
was not called at all if the security state is NONE.

More generally, this feels like a case of "if the function is scoped narrowly to
'do action X', it is more testable than if it is a hook that says 'now do
whatever security state changes you might need to make'".

I am not telling you you have to do something one way or another, just saying
that while I agree with you that the current code is much better than before
your efforts, I suspect the refactoring you mention needing to do to achieve
this might make it even better.  Maybe that shouldn't be for this CL, since it's
a regression fix.  I leave it to your judgment.

> I /think/ the way to actually get the raw styling would entail making
> RenderTextTestApi a class consumable by other tests, then add a new
> IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, URLEmphasisTests) that does
> something like
> 
>   gfx::RenderText* render_text = omnibox_view_views->GetRenderText();
>   RenderTextTestApi text_api = RenderTextTestApi(render_text);
> 
> ... and then uses the RenderTextTestApi to walk over the ranges of text and
> validates that each is as expected. While that might be worthwhile in the
> long-term, I'd like to get this immediate regression fix landed ASAP.

I definitely would not try to make the tests check what text styling we actually
use :).  Not only does that seem like a lot of plumbing, it sounds a bit change
detector-y to me.

https://codereview.chromium.org/2734783007/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2734783007/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:578: security_level_ ==
security_state::HTTP_SHOW_WARNING)
The addition of HTTP_SHOW_WARNING here is a views behavior change, I think.  I'm
glad we match Mac now; I'm curious what the visible effect of this is and how to
know whether having views match Mac (rather than Mac matching views) is correct.

[elawrence, 2017-03-09 16:53:26]

> I am not telling you you have to do something one way or another, just saying
> that while I agree with you that the current code is much better than before
> your efforts, I suspect the refactoring you mention needing to do to achieve
> this might make it even better.  Maybe that shouldn't be for this CL, since
it's
> a regression fix.  I leave it to your judgment.

Thanks. I think it makes sense to fix the regression as quickly as possible.

Longer term, it does make sense to roundtrip the SecurityLevel up through the
UpdateTextStyle function if all of the clients are expected to behave the same
way as they do now. If we do that roundtrip, there's a bunch of other cleanup
that would make sense (e.g. making Mac and Views get/cache the SecurityLevel the
same way), and we also probably need to be using the SecurityInfo (a superset of
SecurityLevel) in order to fix crbug.com/659725.

> I definitely would not try to make the tests check what text styling we
actually
> use :).  Not only does that seem like a lot of plumbing, it sounds a bit
change
> detector-y to me.

Ah. Querying the RenderText for its styling feels like a reasonable approach to
me (if a bit expensive in that it's per UI framework) but the fact that we
haven't done this to date is one clue that I'm wrong.

https://codereview.chromium.org/2734783007/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:578: security_level_ ==
> security_state::HTTP_SHOW_WARNING)
> The addition of HTTP_SHOW_WARNING here is a views behavior change, I think. 
I'm
> glad we match Mac now; I'm curious what the visible effect of this is and how
to
> know whether having views match Mac (rather than Mac matching views) is
correct.

The introduction of the HTTP_SHOW_WARNING state was a bit ad-hoc and I believe
that updating Views was simply overlooked. The reason that no one noticed the
Mac/Views mismatch is that the circumstances that would result in a visual
change do not exist today (but could in the future). There is a trivial
performance improvement to including the quick exit (avoiding an extra
SchedulePaint()). 

Long block of text:
HTTP_SHOW_WARNING might've been better named WARN_USER_PAGE_NOT_SECURE or
something, as it's not really about HTTP, it's about the fact that we detected
something about the URL or content on the page that makes us nervous and we want
to show the "NOT SECURE" warning in the security chip. 

That "NOT SECURE" security chip state is NOT expected to affect the visual
display of the URL scheme, and in the dominant case (HTTP) it does not because
we don't display the HTTP scheme text in the omnibox. 

Similarly, we also show "NOT SECURE" for ALL data: urls, and we don't want that
to impact the fact that we emphasize the scheme in the URL. The prior omission
of the quick exit for HTTP_SHOW_WARNING didn't result in a visible behavior
change because the SetEmphasis() function painted the data scheme in black and
then the UpdateSchemeStyle() function would then get called and paint the scheme
in black again.

In the future, if we were to introduce a NOT SECURE warning on, say, about:
pages that contain sensitive forms, omitting the quick exit for
HTTP_SHOW_WARNING would result in incorrect behavior because we would end up
painting the scheme in black instead of grey.

Other non-obvious arcana:

- SecurityLevel::SECURE_WITH_POLICY_INSTALLED_CERT is a state that only exists
for ChromeOS, so it's only in the Views implementation. 
- SecurityLevel::SECURITY_WARNING is a deprecated state that is being removed
from the codebase via a different CL.

[elawrence, 2017-03-09 16:53:31]

The CQ bit was checked by elawrence@chromium.org

[commit-bot: I haz the power, 2017-03-09 16:54:09]

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, 2017-03-09 17:35:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489078411785510,
"parent_rev": "a35b40c351e773c864301ef0eb075a7eca1a2f2f", "commit_rev":
"f079e7398cefd26400234c38a83630d2f5730969"}

[commit-bot: I haz the power, 2017-03-09 17:36:21]

Description was changed from

==========
Update scheme text color in Omnibox only in non-default Security Levels

A recent refactoring dropped a test from the conditional which caused
the Omnibox to paint the scheme text in black in cases where it should
not. Restore the conditional so that the Omnibox only colors the scheme
text in the correct scenarios.

BUG=699222
TEST=Type about:blank in the omnibox and hit enter. "about:" should be
grey and "blank" should be black
==========

to

==========
Update scheme text color in Omnibox only in non-default Security Levels

A recent refactoring dropped a test from the conditional which caused
the Omnibox to paint the scheme text in black in cases where it should
not. Restore the conditional so that the Omnibox only colors the scheme
text in the correct scenarios.

BUG=699222
TEST=Type about:blank in the omnibox and hit enter. "about:" should be
grey and "blank" should be black

Review-Url: https://codereview.chromium.org/2734783007
Cr-Commit-Position: refs/heads/master@{#455783}
Committed:
https://chromium.googlesource.com/chromium/src/+/f079e7398cefd26400234c38a836...
==========

[commit-bot: I haz the power, 2017-03-09 17:36:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f079e7398cefd26400234c38a836...

[Peter Kasting, 2017-03-09 18:21:40]

On 2017/03/09 16:53:26, elawrence wrote:
> Long block of text:
> HTTP_SHOW_WARNING might've been better named WARN_USER_PAGE_NOT_SECURE or
> something, as it's not really about HTTP, it's about the fact that we detected
> something about the URL or content on the page that makes us nervous and we
want
> to show the "NOT SECURE" warning in the security chip. 
> 
> That "NOT SECURE" security chip state is NOT expected to affect the visual
> display of the URL scheme, and in the dominant case (HTTP) it does not because
> we don't display the HTTP scheme text in the omnibox. 
> 
> Similarly, we also show "NOT SECURE" for ALL data: urls, and we don't want
that
> to impact the fact that we emphasize the scheme in the URL. The prior omission
> of the quick exit for HTTP_SHOW_WARNING didn't result in a visible behavior
> change because the SetEmphasis() function painted the data scheme in black and
> then the UpdateSchemeStyle() function would then get called and paint the
scheme
> in black again.
> 
> In the future, if we were to introduce a NOT SECURE warning on, say, about:
> pages that contain sensitive forms, omitting the quick exit for
> HTTP_SHOW_WARNING would result in incorrect behavior because we would end up
> painting the scheme in black instead of grey.

This is a super-awesome explanation.  It addresses the idea that some security
states are more like a "mode" and some more like an "attribute on another mode".

I would love to see a bunch of this captured in the comment in the code there,
since it really gets at the "why" of that early exit in the way I was hoping to
understand.