Profiles: SSLManger broadcasts SSL_INTERNAL_STATE_CHANGED with a NavigationController.

Profiles: SSLManger broadcasts SSL_INTERNAL_STATE_CHANGED with a Source<BrowserContext> instead of a Source<NavigationController>.

We do this since SSLManagers want to hear all the notifications coming from different NavigationControllers; not just from the one they are associated with.

BUG=87457
TEST=none
R=abarth
TBR=darin

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95541

[Elliot Glaysher, 2011-08-02 22:56:13]

I've tested this change by making sure the insecure content infobar still shows.

[eroman, 2011-08-02 23:29:14]

I am not an OWNERS for this code.

FWIW, LGTM.

[Elliot Glaysher, 2011-08-02 23:31:11]

According to content/browser/net/OWNERS, you are. Do you have any
better recommendations? The svn blame log recommends abarth, but he's
not in the OWNERS.

On Tue, Aug 2, 2011 at 4:29 PM,  <eroman@chromium.org> wrote:
> I am not an OWNERS for this code.
>
> FWIW, LGTM.
>
> http://codereview.chromium.org/7542029/
>

[Elliot Glaysher, 2011-08-02 23:36:10]

I am dumb. Switching to Adam for review.

[abarth-chromium, 2011-08-02 23:40:20]

Hum...  I think we need to broadcast this message to all the tabs in the same
profile.  Here's a test you can run:

1) Have two HTML documents hosted on HTTPS in the same origin in two separate
tabs, but in the same renderer process.
2) Click a button that loads an insecure script in one of the HTML documents.
3) Click through the infobar to allow the insecure script.
4) Observer that both tabs show a red slash through their lock icons.

[Elliot Glaysher, 2011-08-02 23:48:36]

On 2011/08/02 23:40:20, abarth wrote:
> Hum...  I think we need to broadcast this message to all the tabs in the same
> profile.  Here's a test you can run:
> 
> 1) Have two HTML documents hosted on HTTPS in the same origin in two separate
> tabs, but in the same renderer process.
> 2) Click a button that loads an insecure script in one of the HTML documents.
> 3) Click through the infobar to allow the insecure script.
> 4) Observer that both tabs show a red slash through their lock icons.

I just tested that and both tabs have a slash through their "https" in both
tabs.

[Elliot Glaysher, 2011-08-02 23:50:09]

Another note: The current usage of NotificationService is just weird. Individual
listeners listen on a specific NavigationController object but the single
Notify() broadcasts to AllSources().

[abarth-chromium, 2011-08-02 23:50:38]

Thanks for checking.  LGTM.

[abarth-chromium, 2011-08-02 23:52:49]

Is there a better design we should use instead?  I might not have set it up
correctly.  Basically, what needs to happen is that when the state changes,
everyone who's got UI based on the state needs to check whether they need to
change their UI (i.e., add the red slash).

[Elliot Glaysher, 2011-08-03 00:05:21]

I just checked again after noticing that I didn't also update the notification
registration, too. After changing that, it broke exactly like you thought it
would.

A question: is it OK to receive SSL_INTERNAL_STATE_CHANGED redundantly?

[abarth-chromium, 2011-08-03 00:11:58]

> A question: is it OK to receive SSL_INTERNAL_STATE_CHANGED redundantly?

Yes.

[abarth-chromium, 2011-08-03 00:11:59]

> A question: is it OK to receive SSL_INTERNAL_STATE_CHANGED redundantly?

Yes.

[Elliot Glaysher, 2011-08-04 21:06:47]

PTAL.

All that I've done is move the notification source from the NavigationController
to the BrowserContext (content's Profile). While they're currently unused, I put
the NavigationController that generated the notification into the details.

I've tested this with your use case and it crosses out the https in multiple
tabs.

[abarth-chromium, 2011-08-04 21:09:32]

Yay!  Thanks.

http://codereview.chromium.org/7542029/diff/6001/content/browser/ssl/ssl_mana...
File content/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/7542029/diff/6001/content/browser/ssl/ssl_mana...
content/browser/ssl/ssl_manager.cc:53:
Details<NavigationController>(controller));
These details shouldn't be needed.  I'd skip it.  We can always add it if
needed.

[abarth-chromium, 2011-08-04 21:10:01]

LGTM (modulo addressing the comment above)

[Elliot Glaysher, 2011-08-04 23:55:36]

TBR=darin (I am only changing the comment in content_notification_types.h)