Generalize WebContentsObserver::SecurityStyleChanged

Generalize WebContentsObserver::SecurityStyleChanged.

This will put the onus on individual observers to extract the security
information they require, after a change has occurred.  The intent is to
allow chrome/browser-residing observers to monitor security level
changes, independent of style.

BUG=641508
Committed: https://crrev.com/6d1d727a5e0aad78d6151820c8b3ae4d6616cf56
Cr-Commit-Position: refs/heads/master@{#437659}

[cjgrant, 2016-12-07 18:51:06]

cjgrant@chromium.org changed reviewers:
+ boliu@chromium.org, estark@chromium.org

[cjgrant, 2016-12-07 18:51:07]

Bo and Emily, could you take a look at this?

There are a few ways to wire the Clank VR omnibox to proper security level. 
This way looks reasonably frictionless.

Emily, you had suggested renaming SecurityStyleChanged(), removing its
arguments, and letting observer implementations pull security style via the
delegate.  This would work, but there's some benefit in not duplicating the
security-style extraction in multiple spots (duplicate code and processing at
runtime).

Alternatively, the VR web contents observer could implement
SecurityStyleChanged() when it cares about visible state changes, but then VR is
making assumptions about the conditions in which SecurityStyleChanged() will be
called.

This way looks clean to me, even though two callbacks are made to each observer.
 Thoughts?

[boliu, 2016-12-07 18:58:03]

https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:311: virtual void
DidChangeVisibleSecurityState() {}
I don't understand why this is needed when SecurityStyleChanged already exists

[cjgrant, 2016-12-07 19:15:53]

https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:311: virtual void
DidChangeVisibleSecurityState() {}
On 2016/12/07 18:58:03, boliu wrote:
> I don't understand why this is needed when SecurityStyleChanged already exists

See my initial comments on the CL.  "Security style" looks to be a digest of the
more detailed security state.  If the callback invocation was "fixed" to
actually only be called when style changes, we'd potentially miss events.  I
think that's why Emily suggested reworking the callback for clarity.

And while reworking the single callback is one option, it's also an option to
have a second callback - one tailored to observers of security style, and
another for Chrome-internal observers who know how to pull more detail out of
the delegate.

[boliu, 2016-12-07 19:53:19]

https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:311: virtual void
DidChangeVisibleSecurityState() {}
On 2016/12/07 19:15:53, cjgrant wrote:
> On 2016/12/07 18:58:03, boliu wrote:
> > I don't understand why this is needed when SecurityStyleChanged already
exists
> 
> See my initial comments on the CL.  "Security style" looks to be a digest of
the
> more detailed security state.  If the callback invocation was "fixed" to
> actually only be called when style changes, we'd potentially miss events.  I
> think that's why Emily suggested reworking the callback for clarity.
> 
> And while reworking the single callback is one option, it's also an option to
> have a second callback - one tailored to observers of security style, and
> another for Chrome-internal observers who know how to pull more detail out of
> the delegate.
> 

I prefer the just one callback. Having two very close but subtly different
callbacks just invites bugs. At least calling things more frequently could only
lead to wasting work, but not incorrect code.

But if estark@ prefers two, then that's fine too. Definitely write a long
comment explaining the difference between these two though.

[cjgrant, 2016-12-07 20:04:53]

On 2016/12/07 19:53:19, boliu wrote:
>
https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
> content/public/browser/web_contents_observer.h:311: virtual void
> DidChangeVisibleSecurityState() {}
> On 2016/12/07 19:15:53, cjgrant wrote:
> > On 2016/12/07 18:58:03, boliu wrote:
> > > I don't understand why this is needed when SecurityStyleChanged already
> exists
> > 
> > See my initial comments on the CL.  "Security style" looks to be a digest of
> the
> > more detailed security state.  If the callback invocation was "fixed" to
> > actually only be called when style changes, we'd potentially miss events.  I
> > think that's why Emily suggested reworking the callback for clarity.
> > 
> > And while reworking the single callback is one option, it's also an option
to
> > have a second callback - one tailored to observers of security style, and
> > another for Chrome-internal observers who know how to pull more detail out
of
> > the delegate.
> > 
> 
> I prefer the just one callback. Having two very close but subtly different
> callbacks just invites bugs. At least calling things more frequently could
only
> lead to wasting work, but not incorrect code.
> 
> But if estark@ prefers two, then that's fine too. Definitely write a long
> comment explaining the difference between these two though.

Makes sense.  Emily preferred one - generalizing the current callback name,
removing its arguments, and letting each observer pull what it wants in its
implementation.

I could also just generalize the name, and leave it returning the information it
currently does.  VisibleSecurityStateChanged(..) was suggested, but conflicts
with a SecurityStateTabHelper method), hence DidChangeVisibleSecurityState().

[boliu, 2016-12-07 21:08:09]

On 2016/12/07 20:04:53, cjgrant wrote:
> On 2016/12/07 19:53:19, boliu wrote:
> >
>
https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
> > File content/public/browser/web_contents_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/2562503002/diff/1/content/public/browser/web_...
> > content/public/browser/web_contents_observer.h:311: virtual void
> > DidChangeVisibleSecurityState() {}
> > On 2016/12/07 19:15:53, cjgrant wrote:
> > > On 2016/12/07 18:58:03, boliu wrote:
> > > > I don't understand why this is needed when SecurityStyleChanged already
> > exists
> > > 
> > > See my initial comments on the CL.  "Security style" looks to be a digest
of
> > the
> > > more detailed security state.  If the callback invocation was "fixed" to
> > > actually only be called when style changes, we'd potentially miss events. 
I
> > > think that's why Emily suggested reworking the callback for clarity.
> > > 
> > > And while reworking the single callback is one option, it's also an option
> to
> > > have a second callback - one tailored to observers of security style, and
> > > another for Chrome-internal observers who know how to pull more detail out
> of
> > > the delegate.
> > > 
> > 
> > I prefer the just one callback. Having two very close but subtly different
> > callbacks just invites bugs. At least calling things more frequently could
> only
> > lead to wasting work, but not incorrect code.
> > 
> > But if estark@ prefers two, then that's fine too. Definitely write a long
> > comment explaining the difference between these two though.
> 
> Makes sense.  Emily preferred one - generalizing the current callback name,
> removing its arguments, and letting each observer pull what it wants in its
> implementation.

That one sgtm

[estark, 2016-12-07 21:15:49]

Yeah, my preference is for one observer method. SecurityStyleChanged only has
one non-test override and was introduced for one purpose only (ferrying security
info to devtools), so I don't think we need to contort ourselves too much to
prevent the one SecurityStyleChanged override from having to ask the delegate
for the information it needs.

That said, though, my understanding from your email is that this whole
WebContentsObserver strategy is temporary and might not even work in the
short-term (b/c VrWebContentsObserver is not allowed to depend on
//chrome/browser/ssl)?

[cjgrant, 2016-12-07 21:43:20]

On 2016/12/07 21:15:49, estark wrote:
> Yeah, my preference is for one observer method. SecurityStyleChanged only has
> one non-test override and was introduced for one purpose only (ferrying
security
> info to devtools), so I don't think we need to contort ourselves too much to
> prevent the one SecurityStyleChanged override from having to ask the delegate
> for the information it needs.
> 
> That said, though, my understanding from your email is that this whole
> WebContentsObserver strategy is temporary and might not even work in the
> short-term (b/c VrWebContentsObserver is not allowed to depend on
> //chrome/browser/ssl)?

Emily, I have your original recommendation coded, and am building the browser
tests.

I have a better handle on the dependency problem now.  It sums up as:

- I would *prefer* that our WebContentsObserver use is permanent.
- Right now, VrShell is part of chrome/browser, but it's a library.  Therefore,
our unit test build will now have to bring in chrome/browser to link properly,
but we can fix that separately.
- Eventually, we want VrShell to be a component.  When that happens, we'll have
to sort out SecurityStateTabHelper.  It's possible some/all of that class should
also be a component - I don't know yet.
- I do *not* want to try and plumb the Clank omnibox security icon path into our
UI, because it will break as soon as we try to run on a desktop.

In summary, WebContentsObserver for the win!

[cjgrant, 2016-12-07 22:58:14]

Folks, PTAL when you can.

I'm assuming the browser tests that check security style changes are still
valid, even though the observer callback is no longer style-specific.

Also, I'd be happy to separate this CL from the VR-shell specific changes, as
they can certainly stand on their own now, if you like.

[cjgrant, 2016-12-07 23:00:11]

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

[commit-bot: I haz the power, 2016-12-07 23:00:29]

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-12-07 23:45:36]

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

[commit-bot: I haz the power, 2016-12-07 23:45:37]

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

[boliu, 2016-12-08 05:11:29]

boliu@chromium.org changed reviewers:
+ dgozman@chromium.org

[boliu, 2016-12-08 05:11:30]

content lgtm

https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/security_handler.cc (right):

https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/security_handler.cc:94:
web_contents()->GetDelegate()->GetSecurityStyle(
there's prior example with ShowCertificateViewerInDevTools below, but this
really is abusing WebContentsDelegate

it's *WebContents*Delegate, only WebContentsImpl should call stuff on it, not
other random things, in content or otherwise

actually in this case, since these two methods that's called *only* by devtools,
there probably should be a separate interface for these

follow up clean up I guess.. +dgozman

[cjgrant, 2016-12-08 16:03:21]

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

[commit-bot: I haz the power, 2016-12-08 16:03:54]

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-12-08 17:08:31]

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

[commit-bot: I haz the power, 2016-12-08 17:08:32]

Dry run: This issue passed the CQ dry run.

[cjgrant, 2016-12-08 17:10:14]

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

[commit-bot: I haz the power, 2016-12-08 17:10:32]

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

[cjgrant, 2016-12-08 17:10:55]

Description was changed from

==========
Wire the VR omnibox directly to visible security state changes.

Rather than querying for security state changes on navigation events,
let WebContentsObserver trigger observers on a state change.

BUG=641508
==========

to

==========
Generalize WebContentsObserver::SecurityStyleChanged.

This will put the onus on individual observers to extract the security
information they require, after a change has occurred.  The intent is to
allow chrome/browser-residing observers to monitor security level
changes, independent of style.

BUG=641508
==========

[commit-bot: I haz the power, 2016-12-08 18:08:14]

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

[commit-bot: I haz the power, 2016-12-08 18:08:14]

Dry run: This issue passed the CQ dry run.

[estark, 2016-12-08 18:11:04]

lgtm

On 2016/12/08 05:11:30, boliu wrote:
> content lgtm
> 
>
https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
> File content/browser/devtools/protocol/security_handler.cc (right):
> 
>
https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/security_handler.cc:94:
> web_contents()->GetDelegate()->GetSecurityStyle(
> there's prior example with ShowCertificateViewerInDevTools below, but this
> really is abusing WebContentsDelegate
> 
> it's *WebContents*Delegate, only WebContentsImpl should call stuff on it, not
> other random things, in content or otherwise
> 
> actually in this case, since these two methods that's called *only* by
devtools,
> there probably should be a separate interface for these
> 
> follow up clean up I guess.. +dgozman

I agree it's kinda gross, but these two devtools methods are far from the only
offenders. WebContents::GetDelegate() should really be protected if other random
things aren't supposed to call stuff on it.

[cjgrant, 2016-12-08 18:19:53]

cjgrant@chromium.org changed reviewers:
+ clamy@chromium.org, mattm@chromium.org

[cjgrant, 2016-12-08 18:19:53]

mattm@chromium.org: Please review changes in 

- safe_browsing_blocking_page_test.cc

clamy@chromium.org: Please review changes in 

- web_contents_observer.h

Thanks!

[dgozman, 2016-12-08 19:07:49]

https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/security_handler.cc (right):

https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/security_handler.cc:94:
web_contents()->GetDelegate()->GetSecurityStyle(
On 2016/12/08 05:11:30, boliu wrote:
> there's prior example with ShowCertificateViewerInDevTools below, but this
> really is abusing WebContentsDelegate
> 
> it's *WebContents*Delegate, only WebContentsImpl should call stuff on it, not
> other random things, in content or otherwise
> 
> actually in this case, since these two methods that's called *only* by
devtools,
> there probably should be a separate interface for these
> 
> follow up clean up I guess.. +dgozman

Feel free to discuss this with content API owners:
https://codereview.chromium.org/1325023002#msg9

[boliu, 2016-12-08 19:18:48]

On 2016/12/08 19:07:49, dgozman wrote:
>
https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
> File content/browser/devtools/protocol/security_handler.cc (right):
> 
>
https://codereview.chromium.org/2562503002/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/security_handler.cc:94:
> web_contents()->GetDelegate()->GetSecurityStyle(
> On 2016/12/08 05:11:30, boliu wrote:
> > there's prior example with ShowCertificateViewerInDevTools below, but this
> > really is abusing WebContentsDelegate
> > 
> > it's *WebContents*Delegate, only WebContentsImpl should call stuff on it,
not
> > other random things, in content or otherwise
> > 
> > actually in this case, since these two methods that's called *only* by
> devtools,
> > there probably should be a separate interface for these
> > 
> > follow up clean up I guess.. +dgozman
> 
> Feel free to discuss this with content API owners:
> https://codereview.chromium.org/1325023002#msg9

jam@ was objecting to putting it in ContentBrowserClient, and I definitely agree
as well.

If there's only an one off, then adding it to WebContentsDelegate is probably
better than creating a new interface. There's two now, so maybe it's a trend now
:p

[mattm, 2016-12-08 21:12:49]

safe_browsing lgtm

[clamy, 2016-12-09 14:25:44]

Thanks! A few comments below.

https://codereview.chromium.org/2562503002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2562503002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1467: for (auto& observer :
observers_) {
nit: remove curly braces, since the condition is now one line long.

https://codereview.chromium.org/2562503002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2562503002/diff/80001/content/public/browser/...
content/public/browser/web_contents_observer.h:300: virtual void
DidChangeVisibleSecurityState() {}
This method does not seem to be used outside of content/ except in unit tests.
Is that normal?

[cjgrant, 2016-12-09 16:08:29]

https://codereview.chromium.org/2562503002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2562503002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1467: for (auto& observer :
observers_) {
On 2016/12/09 14:25:44, clamy wrote:
> nit: remove curly braces, since the condition is now one line long.

Done.

https://codereview.chromium.org/2562503002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2562503002/diff/80001/content/public/browser/...
content/public/browser/web_contents_observer.h:300: virtual void
DidChangeVisibleSecurityState() {}
On 2016/12/09 14:25:44, clamy wrote:
> This method does not seem to be used outside of content/ except in unit tests.
> Is that normal?

I've rebased this on top of CL 2556963006, which landed yesterday.  It shows
what VrShell is trying to achieve (ie. using a WebContentsObserver, but fishing
for security information that content isn't aware of).  I'm still trying to
grasp the detailed boundaries of the content API, and the previous comments on
this review help.  If you think I should be doing this differently, let me know!

[estark, 2016-12-09 16:12:44]

https://codereview.chromium.org/2562503002/diff/80001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/2562503002/diff/80001/content/public/browser/...
content/public/browser/web_contents_observer.h:300: virtual void
DidChangeVisibleSecurityState() {}
On 2016/12/09 14:25:44, clamy wrote:
> This method does not seem to be used outside of content/ except in unit tests.
> Is that normal?

That's normal for WebContentsObserver methods, see e.g. OnWebContentsFocused,
AccessibilityEventReceived, BeforeUnloadFired, UserAgentOverrideSet, etc.

[clamy, 2016-12-09 17:15:36]

Thanks! Lgtm.

[mthiesse, 2016-12-09 17:36:59]

mthiesse@chromium.org changed reviewers:
+ mthiesse@chromium.org

[mthiesse, 2016-12-09 17:37:00]

vr_shell/ lgtm

[cjgrant, 2016-12-09 18:11:40]

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

[commit-bot: I haz the power, 2016-12-09 18:12:31]

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-12-09 20:17:30]

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

[commit-bot: I haz the power, 2016-12-09 20:17:31]

Dry run: This issue passed the CQ dry run.

[cjgrant, 2016-12-09 20:52:49]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2016-12-09 20:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
estark@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2562503002/#ps120001
(title: "Fix nit.")

[commit-bot: I haz the power, 2016-12-09 20:54:00]

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-12-09 21:17:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481316769485540,
"parent_rev": "a8b3ed12f931c01cdc469e703becbeefb70da49c", "commit_rev":
"f5076083f6cabfb0e02c73104b85ec02ba7b2da7"}

[commit-bot: I haz the power, 2016-12-09 21:17:41]

Description was changed from

==========
Generalize WebContentsObserver::SecurityStyleChanged.

This will put the onus on individual observers to extract the security
information they require, after a change has occurred.  The intent is to
allow chrome/browser-residing observers to monitor security level
changes, independent of style.

BUG=641508
==========

to

==========
Generalize WebContentsObserver::SecurityStyleChanged.

This will put the onus on individual observers to extract the security
information they require, after a change has occurred.  The intent is to
allow chrome/browser-residing observers to monitor security level
changes, independent of style.

BUG=641508

Review-Url: https://codereview.chromium.org/2562503002
==========

[commit-bot: I haz the power, 2016-12-09 21:17:42]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-12 14:41:07]

Description was changed from

==========
Generalize WebContentsObserver::SecurityStyleChanged.

This will put the onus on individual observers to extract the security
information they require, after a change has occurred.  The intent is to
allow chrome/browser-residing observers to monitor security level
changes, independent of style.

BUG=641508

Review-Url: https://codereview.chromium.org/2562503002
==========

to

==========
Generalize WebContentsObserver::SecurityStyleChanged.

This will put the onus on individual observers to extract the security
information they require, after a change has occurred.  The intent is to
allow chrome/browser-residing observers to monitor security level
changes, independent of style.

BUG=641508

Committed: https://crrev.com/6d1d727a5e0aad78d6151820c8b3ae4d6616cf56
Cr-Commit-Position: refs/heads/master@{#437659}
==========

[commit-bot: I haz the power, 2016-12-12 14:41:08]

Patchset 7 (id:??) landed as
https://crrev.com/6d1d727a5e0aad78d6151820c8b3ae4d6616cf56
Cr-Commit-Position: refs/heads/master@{#437659}