content: convert SSL notifications to observer usage

content: convert SSL notifications to observer usage

BUG=170921
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181488

[jam, 2013-01-24 02:02:19]

https://codereview.chromium.org/12041059/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/12041059/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:242: virtual void
NotifyVisibleSSLStateChanged() = 0;
chrome shouldn't have to call into content to notify itself of something.  so
the code in chrome that's calling this should do it a different way, and this
method shouldn't be public.

[Paweł Hajdan Jr., 2013-01-24 18:53:43]

https://codereview.chromium.org/12041059/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/12041059/diff/1/content/public/browser/web_co...
content/public/browser/web_contents.h:242: virtual void
NotifyVisibleSSLStateChanged() = 0;
On 2013/01/24 02:02:19, John Abd-El-Malek wrote:
> chrome shouldn't have to call into content to notify itself of something.  so
> the code in chrome that's calling this should do it a different way, and this
> method shouldn't be public.

Done.

https://codereview.chromium.org/12041059/diff/5001/content/browser/ssl/ssl_ma...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/12041059/diff/5001/content/browser/ssl/ssl_ma...
content/browser/ssl/ssl_manager.cc:131: UpdateEntry(
Question: I'd need to broadcast the notification to all other SSLManagers in the
same BrowserContext (this is needed to get
SSLUITest.TestRunsInsecureContentTwoTabs test to pass).

Do you have any suggestions how to do that?

[jam, 2013-01-30 01:17:51]

https://codereview.chromium.org/12041059/diff/5001/content/browser/ssl/ssl_ma...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/12041059/diff/5001/content/browser/ssl/ssl_ma...
content/browser/ssl/ssl_manager.cc:131: UpdateEntry(
On 2013/01/24 18:53:43, Paweł Hajdan Jr. wrote:
> Question: I'd need to broadcast the notification to all other SSLManagers in
the
> same BrowserContext (this is needed to get
> SSLUITest.TestRunsInsecureContentTwoTabs test to pass).
> 
> Do you have any suggestions how to do that?

there isn't a way to do this now. one suggestion:

BrowserContext derives from SupportsUserData. Perhaps internally in content we
add an object to it that can hold a list of callbacks, and that object can
handle addition/removal of callbacks. it can also have a method that SSLManagers
can call to notify the other callbackst hat internal state change.

all this code can even be inside ssl_manager.cc, since no other class should
know/care about this.

[Paweł Hajdan Jr., 2013-02-05 12:49:29]

PTAL - this should be ready for review now.

If more people should look at this, please just add them directly as reviewers
to decrease review latency (as opposed to e.g. telling me to add them as
reviewers).

[jam, 2013-02-07 17:56:23]

lgtm, sorry for the delay I just saw this. in the future feel free to IM me if i
dont respond soon.

https://codereview.chromium.org/12041059/diff/13001/chrome/browser/ssl/ssl_ta...
File chrome/browser/ssl/ssl_tab_helper.h (right):

https://codereview.chromium.org/12041059/diff/13001/chrome/browser/ssl/ssl_ta...
chrome/browser/ssl/ssl_tab_helper.h:25: public
content::WebContentsUserData<SSLTabHelper> {
nit: should line up with public on previous line. if it doesn't fit, move the
previous one down. also indentation should be 4 spaces

https://codereview.chromium.org/12041059/diff/13001/content/browser/ssl/ssl_m...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/12041059/diff/13001/content/browser/ssl/ssl_m...
content/browser/ssl/ssl_manager.cc:90: if (!managers)
can this really happen? if not, i prefer not adding this.

https://codereview.chromium.org/12041059/diff/13001/content/browser/ssl/ssl_m...
content/browser/ssl/ssl_manager.cc:129: if (managers)
ditto, don't see how this could happen

[Paweł Hajdan Jr., 2013-02-08 09:12:22]

https://codereview.chromium.org/12041059/diff/13001/chrome/browser/ssl/ssl_ta...
File chrome/browser/ssl/ssl_tab_helper.h (right):

https://codereview.chromium.org/12041059/diff/13001/chrome/browser/ssl/ssl_ta...
chrome/browser/ssl/ssl_tab_helper.h:25: public
content::WebContentsUserData<SSLTabHelper> {
On 2013/02/07 17:56:24, jam wrote:
> nit: should line up with public on previous line. if it doesn't fit, move the
> previous one down. also indentation should be 4 spaces

Done.

https://codereview.chromium.org/12041059/diff/13001/content/browser/ssl/ssl_m...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/12041059/diff/13001/content/browser/ssl/ssl_m...
content/browser/ssl/ssl_manager.cc:90: if (!managers)
On 2013/02/07 17:56:24, jam wrote:
> can this really happen? if not, i prefer not adding this. 

Done.

https://codereview.chromium.org/12041059/diff/13001/content/browser/ssl/ssl_m...
content/browser/ssl/ssl_manager.cc:129: if (managers)
On 2013/02/07 17:56:24, jam wrote:
> ditto, don't see how this could happen

Done.

[ppi, 2013-02-12 11:17:16]

Hi,

Could you guys give me a hint on why do you store the list of ssl managers using
user data, and not just as a field in BrowserContext? Am I missing something?

[Paweł Hajdan Jr., 2013-02-13 13:09:29]

On 2013/02/12 11:17:16, ppi wrote:
> Hi,
> 
> Could you guys give me a hint on why do you store the list of ssl managers
using
> user data, and not just as a field in BrowserContext? Am I missing something?

I think the idea is to limit the dependencies/coupling between BrowserContext
and other parts of content.